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

chore(ToolbarMenuItem | Dropdown | DropdownItem): remove hard icon dependencies #12164

Merged
merged 10 commits into from
Mar 4, 2020

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Mar 3, 2020

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

This PR removes the internal usage of the Icon component inside the Dropdown, DropdownItem, ToolbarMenuItem components for the indicators props. The icon that was previously rendered as indicator, is now added as part of the styles in the theme. See more details regarding this effort on #12145

BREAKING CHANGE

  • Changed submenuIndicator and activeIndicator props in ToolbarMenu and ToolbarMenuItem components to be Box shorthand instead of Icon
  • Changed clearIndicator, toggleIndicator and checkableIndicator props in Dropdown and DropdownItem components to be Box shorthand instead of Icon
Microsoft Reviewers: Open in CodeFlow

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Mar 3, 2020

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 678 671
BaseButton (experiments) 885 892
DefaultButton 890 958
DefaultButton (experiments) 1773 1751
DetailsRow 3207 3202
DetailsRow (fast icons) 3197 3202
DetailsRow without styles 2983 2954
DocumentCardTitle with truncation 1639 1653
MenuButton 1255 1261
MenuButton (experiments) 3378 3348
PrimaryButton 1108 1091
PrimaryButton (experiments) 1848 1824
SplitButton 2638 2684
SplitButton (experiments) 6648 6681
Stack 423 429
Stack with Intrinsic children 1036 1027
Stack with Text children 3755 3759
Text 344 337
Toggle 777 772
Toggle (experiments) 2119 2121
button 55 62

Perf Analysis (Fluent)

⚠️ 2 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
Dropdown.Fluent 3593 3743 0.96:1 analysis
DropdownMinimalPerf.default 3540 3828 0.92:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.48 0.47 1.02:1 2000 968
🦄 Button.Fluent 0.15 0.23 0.65:1 1000 149
🔧 Checkbox.Fluent 0.88 0.35 2.51:1 1000 882
🔧 Dialog.Fluent 0.36 0.21 1.71:1 5000 1796
🔧 Dropdown.Fluent 3.59 0.46 7.8:1 1000 3593
🔧 Icon.Fluent 0.16 0.05 3.2:1 5000 775
🦄 Image.Fluent 0.06 0.1 0.6:1 5000 298
🔧 Slider.Fluent 1.58 0.43 3.67:1 1000 1576
🔧 Text.Fluent 0.07 0.02 3.5:1 5000 343
🦄 Tooltip.Fluent 0.11 18.26 0.01:1 5000 574

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
Button.Fluent 149 136 1.1:1
BoxMinimalPerf.default 320 296 1.08:1
AlertMinimalPerf.default 635 591 1.07:1
FlexMinimalPerf.default 272 254 1.07:1
InputMinimalPerf.default 1121 1047 1.07:1
DropdownManyItemsPerf.default 353 332 1.06:1
SliderMinimalPerf.default 1628 1551 1.05:1
LayoutMinimalPerf.default 669 642 1.04:1
RefMinimalPerf.default 200 193 1.04:1
Text.Fluent 343 329 1.04:1
Tooltip.Fluent 574 554 1.04:1
AnimationMinimalPerf.default 637 617 1.03:1
CheckboxMinimalPerf.default 4312 4198 1.03:1
LabelMinimalPerf.default 343 333 1.03:1
PopupMinimalPerf.default 419 407 1.03:1
ChatMinimalPerf.default 545 532 1.02:1
ChatWithPopoverPerf.default 622 609 1.02:1
DividerMinimalPerf.default 979 960 1.02:1
HeaderSlotsPerf.default 1677 1646 1.02:1
ListMinimalPerf.default 399 390 1.02:1
ListNestedPerf.default 892 875 1.02:1
LoaderMinimalPerf.default 1097 1075 1.02:1
ProviderMergeThemesPerf.default 1360 1331 1.02:1
RadioGroupMinimalPerf.default 544 534 1.02:1
ReactionMinimalPerf.default 2565 2506 1.02:1
TableMinimalPerf.default 644 629 1.02:1
VideoMinimalPerf.default 870 857 1.02:1
ChatDuplicateMessagesPerf.default 424 419 1.01:1
EmbedMinimalPerf.default 6442 6372 1.01:1
HeaderMinimalPerf.default 529 525 1.01:1
ItemLayoutMinimalPerf.default 2043 2026 1.01:1
ListCommonPerf.default 960 948 1.01:1
ListWith60ListItems.default 180 178 1.01:1
MenuButtonMinimalPerf.default 1887 1862 1.01:1
ProviderMinimalPerf.default 675 670 1.01:1
TextAreaMinimalPerf.default 3246 3202 1.01:1
Icon.Fluent 775 771 1.01:1
Slider.Fluent 1576 1567 1.01:1
AttachmentSlotsPerf.default 3639 3631 1:1
CarouselMinimalPerf.default 2089 2080 1:1
DialogMinimalPerf.default 1811 1809 1:1
FormMinimalPerf.default 911 912 1:1
SegmentMinimalPerf.default 1077 1077 1:1
SplitButtonMinimalPerf.default 12896 12916 1:1
TooltipMinimalPerf.default 844 843 1:1
Dialog.Fluent 1796 1794 1:1
Image.Fluent 298 299 1:1
GridMinimalPerf.default 822 827 0.99:1
PortalMinimalPerf.default 283 285 0.99:1
TextMinimalPerf.default 327 329 0.99:1
AccordionMinimalPerf.default 247 251 0.98:1
AvatarMinimalPerf.default 559 568 0.98:1
ButtonSlotsPerf.default 732 747 0.98:1
MenuMinimalPerf.default 2048 2081 0.98:1
StatusMinimalPerf.default 301 307 0.98:1
CustomToolbarPrototype.default 3851 3937 0.98:1
ToolbarMinimalPerf.default 1081 1101 0.98:1
TreeMinimalPerf.default 1150 1170 0.98:1
IconMinimalPerf.default 365 378 0.97:1
Avatar.Fluent 968 994 0.97:1
Checkbox.Fluent 882 906 0.97:1
AttachmentMinimalPerf.default 949 993 0.96:1
ButtonMinimalPerf.default 129 137 0.94:1
HierarchicalTreeMinimalPerf.default 954 1013 0.94:1
ImageMinimalPerf.default 278 301 0.92:1
TreeWith60ListItems.default 214 236 0.91:1

@size-auditor
Copy link

size-auditor bot commented Mar 3, 2020

Asset size changes

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

Baseline commit: d57d727f68a128f16a5574048b84aecd8cd2aad7 (build)

@mnajdova mnajdova merged commit 5740b17 into microsoft:master Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants