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

fix(Tree): Match design spec to show/hide selectable indicator #15133

Merged
merged 29 commits into from
Oct 19, 2020
Merged

fix(Tree): Match design spec to show/hide selectable indicator #15133

merged 29 commits into from
Oct 19, 2020

Conversation

assuncaocharles
Copy link
Contributor

@assuncaocharles assuncaocharles commented Sep 19, 2020 •
edited
Loading

Pull request checklist

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

BREAKING CHANGES

In master when we introduced the selectable feat for Tree, we were only showing indicator if it's expanded. #12362
Lately, as a fix for #14720 we merged #14916

None of this behaviors match the design spec that says:

  • Indicator is shown on hover and hidden on blur
  • Selectable parent always show indicator when it's expanded.

This PR introduces the behavior described above

ANcx1OuK15

This PR also makes all items ( parent and child ) selectable by default in a selectable Tree.

Focus areas to test

Indicator should alway appear on Mouse Hover and Focus and hide on Mouse Leave and Blur

@msft-github-bot msft-github-bot added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Sep 19, 2020
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 142f5ef:

Sandbox Source
Fluent UI Button Configuration
microsoft/fluentui: codesandbox-react-template Configuration
microsoft/fluentui: codesandbox-react-next-template Configuration
microsoft/fluentui: codesandbox-react-northstar-template Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 19, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6499841:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@size-auditor
Copy link

size-auditor bot commented Sep 19, 2020 •
edited
Loading

Asset size changes

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

Baseline commit: 5158b5e4050cb04e9c22e0d5292cd96cc674746e (build)

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Sep 20, 2020 •
edited
Loading

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 827 850 5000
BaseButtonCompat mount 911 932 5000
Breadcrumb mount 150147 150547 5000
Checkbox mount 1556 1575 5000
CheckboxBase mount 1324 1316 5000
ChoiceGroup mount 4805 4821 5000
ComboBox mount 1014 923 1000
CommandBar mount 20950 21048 1000
ContextualMenu mount 5880 5918 1000
DefaultButtonCompat mount 1147 1131 5000
DetailsRow mount 3679 3726 5000
DetailsRowFast mount 3690 3741 5000
DetailsRowNoStyles mount 3490 3527 5000
Dialog mount 1536 1474 1000
DocumentCardTitle mount 1761 1735 1000
Dropdown mount 3786 3864 5000
FocusTrapZone mount 1799 1782 5000
FocusZone mount 1792 1804 5000
IconButtonCompat mount 1800 1824 5000
Label mount 337 327 5000
Layer mount 1809 1866 5000
Link mount 461 465 5000
MenuButtonCompat mount 1506 1536 5000
MessageBar mount 2041 1989 5000
Nav mount 3287 3321 1000
OverflowSet mount 1018 1042 5000
Panel mount 1427 1424 1000
Persona mount 863 876 1000
Pivot mount 1374 1384 1000
PrimaryButtonCompat mount 1286 1314 5000
Rating mount 7768 7811 5000
SearchBox mount 1360 1363 5000
Shimmer mount 2667 2644 5000
Slider mount 1935 1919 5000
SpinButton mount 5109 5167 5000
Spinner mount 417 419 5000
SplitButtonCompat mount 3197 3216 5000
Stack mount 531 543 5000
StackWithIntrinsicChildren mount 1858 1846 5000
StackWithTextChildren mount 5062 5063 5000
SwatchColorPicker mount 10166 10416 5000
TagPicker mount 2766 2730 5000
TeachingBubble mount 11367 11360 5000
Text mount 431 422 5000
TextField mount 1399 1391 5000
ThemeProvider mount 1932 1914 5000
ThemeProvider virtual-rerender 622 632 5000
Toggle mount 827 820 5000
button mount 547 549 5000
buttonNative mount 106 104 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.47 0.51 0.92:1 2000 939
🦄 Button.Fluent 0.12 0.2 0.6:1 5000 609
🔧 Checkbox.Fluent 0.63 0.37 1.7:1 1000 631
🎯 Dialog.Fluent 0.17 0.22 0.77:1 5000 826
🔧 Dropdown.Fluent 2.89 0.44 6.57:1 1000 2886
🔧 Icon.Fluent 0.15 0.06 2.5:1 5000 761
🦄 Image.Fluent 0.08 0.13 0.62:1 5000 401
🔧 Slider.Fluent 1.56 0.45 3.47:1 1000 1564
🔧 Text.Fluent 0.08 0.03 2.67:1 5000 392
🦄 Tooltip.Fluent 0.12 0.88 0.14:1 5000 578

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ButtonMinimalPerf.default 205 180 1.14:1
RefMinimalPerf.default 247 224 1.1:1
FlexMinimalPerf.default 334 313 1.07:1
ListMinimalPerf.default 532 499 1.07:1
VideoMinimalPerf.default 701 653 1.07:1
ButtonSlotsPerf.default 649 615 1.06:1
DividerMinimalPerf.default 418 394 1.06:1
PortalMinimalPerf.default 172 162 1.06:1
TreeWith60ListItems.default 209 198 1.06:1
CarouselMinimalPerf.default 486 463 1.05:1
RadioGroupMinimalPerf.default 483 462 1.05:1
TreeMinimalPerf.default 950 901 1.05:1
ChatWithPopoverPerf.default 484 466 1.04:1
FormMinimalPerf.default 496 476 1.04:1
LayoutMinimalPerf.default 443 425 1.04:1
MenuMinimalPerf.default 902 870 1.04:1
TextMinimalPerf.default 386 372 1.04:1
Avatar.Fluent 939 902 1.04:1
Text.Fluent 392 377 1.04:1
Tooltip.Fluent 578 556 1.04:1
ButtonUseCssPerf.default 862 835 1.03:1
ButtonUseCssNestingPerf.default 1099 1063 1.03:1
ListWith60ListItems.default 970 945 1.03:1
LoaderMinimalPerf.default 762 737 1.03:1
ReactionMinimalPerf.default 450 439 1.03:1
StatusMinimalPerf.default 788 762 1.03:1
TextAreaMinimalPerf.default 546 531 1.03:1
TooltipMinimalPerf.default 838 812 1.03:1
ChatMinimalPerf.default 661 645 1.02:1
EmbedMinimalPerf.default 2018 1987 1.02:1
IconMinimalPerf.default 746 728 1.02:1
TableManyItemsPerf.default 2336 2294 1.02:1
Slider.Fluent 1564 1527 1.02:1
BoxMinimalPerf.default 400 396 1.01:1
ButtonOverridesMissPerf.default 1710 1697 1.01:1
CardMinimalPerf.default 609 601 1.01:1
DropdownManyItemsPerf.default 792 782 1.01:1
CustomToolbarPrototype.default 3743 3697 1.01:1
Dropdown.Fluent 2886 2853 1.01:1
Icon.Fluent 761 750 1.01:1
AlertMinimalPerf.default 328 328 1:1
AttachmentSlotsPerf.default 1176 1181 1:1
AvatarMinimalPerf.default 481 481 1:1
DropdownMinimalPerf.default 2863 2874 1:1
GridMinimalPerf.default 376 376 1:1
ImageMinimalPerf.default 420 422 1:1
ItemLayoutMinimalPerf.default 1323 1329 1:1
LabelMinimalPerf.default 445 443 1:1
ListNestedPerf.default 612 615 1:1
PopupMinimalPerf.default 703 701 1:1
SplitButtonMinimalPerf.default 3870 3870 1:1
Checkbox.Fluent 631 631 1:1
Dialog.Fluent 826 823 1:1
AccordionMinimalPerf.default 150 151 0.99:1
CheckboxMinimalPerf.default 2827 2865 0.99:1
DialogMinimalPerf.default 805 814 0.99:1
MenuButtonMinimalPerf.default 1641 1651 0.99:1
ProviderMergeThemesPerf.default 1960 1981 0.99:1
ProviderMinimalPerf.default 971 979 0.99:1
SkeletonMinimalPerf.default 447 453 0.99:1
TableMinimalPerf.default 444 450 0.99:1
ToolbarMinimalPerf.default 988 995 0.99:1
Button.Fluent 609 616 0.99:1
AnimationMinimalPerf.default 421 430 0.98:1
ChatDuplicateMessagesPerf.default 418 427 0.98:1
HeaderSlotsPerf.default 839 860 0.98:1
InputMinimalPerf.default 1274 1294 0.98:1
SliderMinimalPerf.default 1525 1551 0.98:1
Image.Fluent 401 410 0.98:1
AttachmentMinimalPerf.default 170 175 0.97:1
HeaderMinimalPerf.default 400 414 0.97:1
ListCommonPerf.default 672 700 0.96:1
SegmentMinimalPerf.default 380 398 0.95:1

@assuncaocharles assuncaocharles mentioned this pull request Sep 21, 2020
Merged
@assuncaocharles assuncaocharles marked this pull request as ready for review September 25, 2020 05:36
@assuncaocharles @layershifter
Update packages/fluentui/react-northstar/src/themes/teams/components/…
675d6dd 
…Tree/treeTitleStyles.ts

Co-authored-by: Oleksandr Fediashov <[email protected]>
@assuncaocharles
Copy link
Contributor Author

@pompomon @layershifter Checked all screen changes, seems correct for me.

@assuncaocharles
Copy link
Contributor Author

@layershifter @pompomon @miroslavstastny please, check this one

@layershifter
Copy link
Member

In master when we introduced the selectable feat for Tree,

Was this released in 0.51.0? If no, it's not a breaking change for customers.

@assuncaocharles
Copy link
Contributor Author

In master when we introduced the selectable feat for Tree,

Was this released in 0.51.0? If no, it's not a breaking change for customers.

@layershifter We are breaking what we introduced in 0.49 in this PR #12362

@assuncaocharles assuncaocharles merged commit 3346b09 into microsoft:master Oct 19, 2020
@assuncaocharles assuncaocharles deleted the fix/tree-selectable branch October 19, 2020 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ridvanovskyy Ridvanovskyy Ridvanovskyy approved these changes

@layershifter layershifter layershifter approved these changes

@kolaps33 kolaps33 Awaiting requested review from kolaps33

@pompomon pompomon Awaiting requested review from pompomon

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@dzearing dzearing Awaiting requested review from dzearing

@ecraig12345 ecraig12345 Awaiting requested review from ecraig12345

@Jahnp Jahnp Awaiting requested review from Jahnp

Assignees

@miroslavstastny miroslavstastny

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.
5 participants
@assuncaocharles @msft-github-bot @layershifter @Ridvanovskyy @miroslavstastny