Skip to content

Conversation

@JasonGore
Copy link
Member

@JasonGore JasonGore commented May 13, 2019

Pull request checklist

Description of changes

Changes createComponent API from:

createComponent({ options });

to:

createComponent(view, { options });

Making options truly optional and view the only required arg.

It is now possible to use createComponent like this:

createComponent(ButtonView);

While allowing options to be added for further customization.

I had also planned on addressing #8331 as part of this PR but since my shield rotation moved up I'm going to make a change just for this for now.

Microsoft Reviewers: Open in CodeFlow

@msft-github-bot
Copy link
Contributor

msft-github-bot commented May 13, 2019

Component perf results:

Scenario Target branch avg total (ms) PR avg total (ms) Target branch avg per item (ms) PR avg per item (ms) Is significant change Is regression
PrimaryButton 94.776 96.268 0.948 0.963 false false
BaseButton 38.694 40.931 0.387 0.409 true true
NewButton 125.259 135.457 1.253 1.355 true true
button 7.156 7.553 0.072 0.075 false false
DetailsRows without styles 216.611 216.966 2.166 2.170 false false
DetailsRows 241.807 247.417 2.418 2.474 false false
Toggles 62.398 62.762 0.624 0.628 false false
NewToggle 80.735 77.064 0.807 0.771 false false
DocumentCardTitle with truncation 32.094 33.295 0.321 0.333 false false

const { factoryOptions = {} } = component;
const { factoryOptions = {} } = options;
const { defaultProp } = factoryOptions;
const displayName = (options && options.displayName) || view.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about this feels weird to me. The fact that the customization can come from either displayName or view.name confuses me a bit, especially because I don't actually see view.name as an explicit property of IViewComponent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name is a JavaScript prototype. This naming change is more symmetric with styled's behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good to know about the name!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good point about using it for customizations though.. we shouldn't use name there because it will be affected by minification and cause confusion. I'll make a quick change to only use options.displayName for Customizations.

@msft-github-bot
Copy link
Contributor

Hello @JasonGore!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull-requests of this repository that have been opened for at least 8 hours, a condition that is not currently met. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me and give me an instruction to get started! Learn more here.

@JasonGore JasonGore merged commit 20b09cc into microsoft:fabric-7 May 13, 2019
@msft-github-bot
Copy link
Contributor

🎉@uifabric/foundation@v0.8.0 has been released which incorporates this pull request.:tada:

Handy links:

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants