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.

Sign up for GitHub

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

Jump to bottom

chore(Tree): Prevent string items #13016

Merged
merged 8 commits into from
May 7, 2020
Merged

chore(Tree): Prevent string items #13016

merged 8 commits into from
May 7, 2020

Conversation

assuncaocharles
Copy link
Contributor

@assuncaocharles assuncaocharles commented May 6, 2020 •
edited by msft-github-bot

Pull request checklist

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

Description of changes

This PR reverts the solution from #12984 and prevents strings as Tree Items

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@msft-github-bot msft-github-bot added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label May 6, 2020
@msft-github-bot
Copy link
Contributor

msft-github-bot commented May 6, 2020 •
edited

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 641 592 5000
Checkbox 1274 1274 5000
CheckboxBase 1072 1031 5000
ChoiceGroup 4012 3895 5000
ComboBox 745 727 1000
CommandBar 6244 6243 1000
ContextualMenu 12149 12081 1000
DefaultButton 842 904 5000
DetailsRow 2781 2824 5000
DetailsRow (fast icons) 2696 2838 5000
DetailsRow without styles 2609 2554 5000
Dialog 1170 1181 1000
DocumentCardTitle with truncation 1431 1396 1000
Dropdown 1918 1935 5000
FocusZone 1355 1379 5000
IconButton 1365 1386 5000
Label 247 244 5000
Link 347 381 5000
MenuButton 1170 1161 5000
Nav 2519 2600 1000
Panel 1166 1213 1000
Persona 651 722 1000
Pivot 1084 1081 1000
PrimaryButton 1011 959 5000
SearchBox 1032 1000 5000
Slider 1226 1235 5000
Spinner 327 326 5000
SplitButton 2382 2488 5000
Stack 378 395 5000
Stack with Intrinsic children 896 875 5000
Stack with Text children 3335 3274 5000
TagPicker 2276 2242 5000
Text 293 291 5000
TextField 1114 1089 5000
Toggle 705 676 5000
button 55 58 5000

Perf Analysis (Fluent)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
PopupMinimalPerf.default 201 208 0.97:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.38 0.37 1.03:1 2000 752
🦄 Button.Fluent 0.09 0.14 0.64:1 5000 444
🔧 Checkbox.Fluent 0.54 0.29 1.86:1 1000 536
🔧 Dialog.Fluent 0.29 0.17 1.71:1 5000 1446
🔧 Dropdown.Fluent 2.73 0.35 7.8:1 1000 2731
🔧 Icon.Fluent 0.12 0.04 3:1 5000 577
🦄 Image.Fluent 0.06 0.09 0.67:1 5000 280
🔧 Slider.Fluent 1.16 0.28 4.14:1 1000 1161
🔧 Text.Fluent 0.06 0.02 3:1 5000 275
🦄 Tooltip.Fluent 0.07 14.82 0:1 5000 374

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
LabelMinimalPerf.default 353 308 1.15:1
BoxMinimalPerf.default 293 258 1.14:1
RefMinimalPerf.default 184 165 1.12:1
ButtonMinimalPerf.default 146 135 1.08:1
ListWith60ListItems.default 930 871 1.07:1
TextAreaMinimalPerf.default 389 366 1.06:1
DividerMinimalPerf.default 278 265 1.05:1
GridMinimalPerf.default 548 528 1.04:1
PortalMinimalPerf.default 253 243 1.04:1
TextMinimalPerf.default 291 279 1.04:1
Dialog.Fluent 1446 1396 1.04:1
Text.Fluent 275 265 1.04:1
CardMinimalPerf.default 443 430 1.03:1
FlexMinimalPerf.default 225 219 1.03:1
FormMinimalPerf.default 598 581 1.03:1
ListCommonPerf.default 819 795 1.03:1
CustomToolbarPrototype.default 3055 2980 1.03:1
Button.Fluent 444 432 1.03:1
Tooltip.Fluent 374 362 1.03:1
DialogMinimalPerf.default 1424 1402 1.02:1
DropdownMinimalPerf.default 2827 2773 1.02:1
LayoutMinimalPerf.default 438 431 1.02:1
ListMinimalPerf.default 377 369 1.02:1
StatusMinimalPerf.default 513 501 1.02:1
TableMinimalPerf.default 304 297 1.02:1
TreeMinimalPerf.default 970 947 1.02:1
Icon.Fluent 577 563 1.02:1
CarouselMinimalPerf.default 388 384 1.01:1
ChatDuplicateMessagesPerf.default 340 338 1.01:1
ChatWithPopoverPerf.default 493 489 1.01:1
HeaderMinimalPerf.default 397 393 1.01:1
HierarchicalTreeMinimalPerf.default 783 774 1.01:1
ImageMinimalPerf.default 298 296 1.01:1
CheckboxMinimalPerf.default 2452 2441 1:1
DropdownManyItemsPerf.default 1139 1143 1:1
EmbedMinimalPerf.default 1646 1645 1:1
InputMinimalPerf.default 848 852 1:1
ItemLayoutMinimalPerf.default 1338 1334 1:1
ListNestedPerf.default 718 721 1:1
LoaderMinimalPerf.default 631 630 1:1
SegmentMinimalPerf.default 742 739 1:1
Slider.Fluent 1161 1164 1:1
AccordionMinimalPerf.default 116 117 0.99:1
ButtonSlotsPerf.default 495 498 0.99:1
ProviderMinimalPerf.default 533 538 0.99:1
ReactionMinimalPerf.default 1541 1559 0.99:1
TooltipMinimalPerf.default 569 577 0.99:1
Avatar.Fluent 752 762 0.99:1
AttachmentSlotsPerf.default 911 927 0.98:1
MenuMinimalPerf.default 1490 1515 0.98:1
SplitButtonMinimalPerf.default 2781 2827 0.98:1
ChatMinimalPerf.default 493 506 0.97:1
ProviderMergeThemesPerf.default 1368 1405 0.97:1
HeaderSlotsPerf.default 1150 1201 0.96:1
MenuButtonMinimalPerf.default 1251 1299 0.96:1
RadioGroupMinimalPerf.default 447 467 0.96:1
TreeWith60ListItems.default 171 178 0.96:1
AnimationMinimalPerf.default 493 518 0.95:1
SliderMinimalPerf.default 1172 1228 0.95:1
IconMinimalPerf.default 508 532 0.95:1
ToolbarMinimalPerf.default 610 644 0.95:1
VideoMinimalPerf.default 489 513 0.95:1
Dropdown.Fluent 2731 2875 0.95:1
Checkbox.Fluent 536 571 0.94:1
Image.Fluent 280 299 0.94:1
AttachmentMinimalPerf.default 117 129 0.91:1
AvatarMinimalPerf.default 414 458 0.9:1
AlertMinimalPerf.default 237 271 0.87:1

@size-auditor
Copy link

size-auditor bot commented May 6, 2020 •
edited

Asset size changes

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

Baseline commit: 5b327a08fdc92dec59f2a201702d02da81696c2e (build)

@assuncaocharles
Copy link
Contributor Author

Is it ok now? @mnajdova and @layershifter

mnajdova
mnajdova approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Please add a fix entry in the changelog

@assuncaocharles assuncaocharles merged commit 8a841d3 into microsoft:master May 7, 2020
@assuncaocharles assuncaocharles deleted the fix/tree-item-strings branch May 7, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnajdova mnajdova mnajdova approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@dzearing dzearing Awaiting requested review from dzearing

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@levithomason levithomason Awaiting requested review from levithomason

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

@pompomon pompomon Awaiting requested review from pompomon

@layershifter layershifter Awaiting requested review from layershifter

Assignees

@ecraig12345 ecraig12345

Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@assuncaocharles @msft-github-bot @mnajdova @layershifter @ecraig12345