Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Button: Decoupling ContextualMenu to improve bundlesize #8220

Closed
wants to merge 5 commits into from

Conversation

khmakoto
Copy link
Member

@khmakoto khmakoto commented Mar 6, 2019

Pull request checklist

Description of changes

This PR takes a stab at decoupling ContextualMenu from the new Button rewrite in the experiments package in an effort to improve bundle size. The way that this is being done is by introducing a new ButtonMenu that has a subset of the props in ContextualMenu, consisting of:

  • target
  • onDismiss
  • items
  • directionalHint

A fifth prop not existing in ContextualMenu, menuType, is also added to indicate what type of component the menu is going to be rendered as. The examples have also been updated to work with this change.

The bundle size change for the components is illustrated below:

Before decoupling (Stat: 533.43KB, Parsed: 160.75KB, Gzipped: 45.13KB):
image

After decoupling (Stat: 250.59KB, Parsed: 69.71KB, Gzipped: 20.45KB):
image

Difference in Size Auditor Dashboard: (Props to @aftab-hassan for sending this exactly when I needed it 👍 )
image

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@size-auditor
Copy link

size-auditor bot commented Mar 6, 2019

Size Auditor did not detect a change in bundle size for any component!

});

return <Slots.root {...rest} />;
};
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended to be a legitimate viable default menu option? Are there any examples that use it? (It seem all the examples were converted to use ContextualMenu.)

* Defines what type of component the menu is going to be rendered as.
*/
menuType?: ISlottableReactType<IButtonMenuProps>;
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to duplicate props in IContextualMenuProps? We should be able to depend on types without affecting bundle size.

Copy link
Member

@JasonGore JasonGore left a comment

Choose a reason for hiding this comment

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

I'm concerned about how much of this is "filler" just to avoid using ContextualMenu. I'd like to talk through some other options of what you were considering and what I was envisioning to see if there are any other alternatives. I was really hoping we could use some kind of slot helpers without creating standalone variants for every function, like menuing.

@khmakoto
Copy link
Member Author

Per discussions offline we have chosen to do this via a different approach, which is done in PR #8291.

@khmakoto khmakoto closed this Mar 13, 2019
@khmakoto khmakoto deleted the separateMenu branch March 13, 2019 00:34
@khmakoto khmakoto restored the separateMenu branch March 25, 2019 17:09
@khmakoto khmakoto deleted the separateMenu branch March 25, 2019 17:09
@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.

4 participants