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

feat(Dropdown): support disabled items #12407

Merged
merged 7 commits into from
Mar 25, 2020
Merged

feat(Dropdown): support disabled items #12407

merged 7 commits into from
Mar 25, 2020

Conversation

silviuaavram
Copy link
Contributor

@silviuaavram silviuaavram commented Mar 24, 2020

Pull request checklist

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

Description of changes

Downshift already supports handling disabled as a key in the item object. We need to pass that from our Dropdown to getItemProps in order to make it work.

Fully functional disabled item navigation should be achievable by updating to Downshift https://github.com/downshift-js/downshift/releases/tag/v4.0.1 or above.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Mar 24, 2020

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 834 832
BaseButton (experiments) 1040 959
DefaultButton 1060 1096
DefaultButton (experiments) 1970 1967
DetailsRow 3449 3480
DetailsRow (fast icons) 3431 3409
DetailsRow without styles 3224 3067
DocumentCardTitle with truncation 1574 1537
MenuButton 1409 1458
MenuButton (experiments) 3513 3521
PrimaryButton 1215 1235
PrimaryButton (experiments) 1999 1993
SplitButton 3107 3107
SplitButton (experiments) 6972 6934
Stack 510 474
Stack with Intrinsic children 1104 1139
Stack with Text children 4260 4233
Text 380 397
Toggle 897 893
Toggle (experiments) 2300 2224
button 68 60

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.56 0.5 1.12:1 2000 1115
🦄 Button.Fluent 0.11 0.19 0.58:1 5000 528
🔧 Checkbox.Fluent 0.75 0.41 1.83:1 1000 752
🔧 Dialog.Fluent 0.42 0.21 2:1 5000 2119
🔧 Dropdown.Fluent 3.69 0.51 7.24:1 1000 3694
🔧 Icon.Fluent 0.19 0.05 3.8:1 5000 927
🎯 Image.Fluent 0.08 0.11 0.73:1 5000 385
🔧 Slider.Fluent 1.57 0.46 3.41:1 1000 1569
🔧 Text.Fluent 0.08 0.02 4:1 5000 423
🦄 Tooltip.Fluent 0.13 17.52 0.01:1 5000 637

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
BoxMinimalPerf.default 424 375 1.13:1
LayoutMinimalPerf.default 789 736 1.07:1
PopupMinimalPerf.default 270 253 1.07:1
ChatWithPopoverPerf.default 664 627 1.06:1
TreeMinimalPerf.default 1338 1261 1.06:1
AccordionMinimalPerf.default 275 264 1.04:1
FlexMinimalPerf.default 316 305 1.04:1
HeaderMinimalPerf.default 639 617 1.04:1
RadioGroupMinimalPerf.default 630 608 1.04:1
AvatarMinimalPerf.default 605 587 1.03:1
ImageMinimalPerf.default 395 385 1.03:1
PortalMinimalPerf.default 327 316 1.03:1
SegmentMinimalPerf.default 1218 1177 1.03:1
TextMinimalPerf.default 429 415 1.03:1
TreeWith60ListItems.default 251 243 1.03:1
Dialog.Fluent 2119 2066 1.03:1
ChatMinimalPerf.default 649 634 1.02:1
HierarchicalTreeMinimalPerf.default 1154 1135 1.02:1
ProviderMinimalPerf.default 677 666 1.02:1
RefMinimalPerf.default 205 201 1.02:1
SliderMinimalPerf.default 1602 1569 1.02:1
TableMinimalPerf.default 768 751 1.02:1
TooltipMinimalPerf.default 920 905 1.02:1
AnimationMinimalPerf.default 700 694 1.01:1
ButtonSlotsPerf.default 659 651 1.01:1
CardMinimalPerf.default 420 415 1.01:1
DropdownManyItemsPerf.default 1552 1533 1.01:1
FormMinimalPerf.default 1059 1052 1.01:1
HeaderSlotsPerf.default 1895 1882 1.01:1
ItemLayoutMinimalPerf.default 2327 2300 1.01:1
MenuButtonMinimalPerf.default 1724 1710 1.01:1
TextAreaMinimalPerf.default 3303 3285 1.01:1
AttachmentSlotsPerf.default 3711 3723 1:1
ChatDuplicateMessagesPerf.default 452 454 1:1
ListNestedPerf.default 1055 1054 1:1
ReactionMinimalPerf.default 2600 2602 1:1
SplitButtonMinimalPerf.default 12731 12693 1:1
VideoMinimalPerf.default 972 969 1:1
Avatar.Fluent 1115 1116 1:1
Button.Fluent 528 528 1:1
Dropdown.Fluent 3694 3708 1:1
Icon.Fluent 927 930 1:1
CarouselMinimalPerf.default 2078 2090 0.99:1
CheckboxMinimalPerf.default 3372 3406 0.99:1
DialogMinimalPerf.default 2127 2154 0.99:1
GridMinimalPerf.default 963 971 0.99:1
IconMinimalPerf.default 452 457 0.99:1
LabelMinimalPerf.default 415 421 0.99:1
ListMinimalPerf.default 480 486 0.99:1
LoaderMinimalPerf.default 1085 1099 0.99:1
StatusMinimalPerf.default 692 698 0.99:1
CustomToolbarPrototype.default 3808 3861 0.99:1
Checkbox.Fluent 752 756 0.99:1
Text.Fluent 423 428 0.99:1
Tooltip.Fluent 637 645 0.99:1
AlertMinimalPerf.default 641 656 0.98:1
AttachmentMinimalPerf.default 940 962 0.98:1
DividerMinimalPerf.default 1076 1095 0.98:1
EmbedMinimalPerf.default 5570 5690 0.98:1
ListCommonPerf.default 1128 1149 0.98:1
MenuMinimalPerf.default 2151 2191 0.98:1
ToolbarMinimalPerf.default 1235 1254 0.98:1
Slider.Fluent 1569 1605 0.98:1
ButtonMinimalPerf.default 152 157 0.97:1
DropdownMinimalPerf.default 3693 3797 0.97:1
InputMinimalPerf.default 1108 1141 0.97:1
ListWith60ListItems.default 1273 1306 0.97:1
ProviderMergeThemesPerf.default 1437 1495 0.96:1
Image.Fluent 385 400 0.96:1

if (
!_.isNil(highlightedIndex) &&
this.state.filteredItems.length &&
!this.props.items[highlightedIndex]['disabled']
Copy link
Member

Choose a reason for hiding this comment

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

It will not work with old render callback, but it's already deprecated:

const items = [render => render("foo")];

@@ -867,6 +867,7 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo
...getItemProps({
item,
index,
disabled: !!item['disabled'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
disabled: !!item['disabled'],
disabled: item['disabled'],

Is it really required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

double checked in downshift and it's not. will remove, thanks!

@silviuaavram silviuaavram merged commit 88a6933 into microsoft:master Mar 25, 2020
@silviuaavram silviuaavram deleted the feat/disabled-dropdown-item branch March 25, 2020 18:13
DuanShaolong pushed a commit to DuanShaolong/fluentui that referenced this pull request Apr 27, 2020
* add disabled prop to DropdownItem

* use disabled prop in downshift getItemProps

* prevent selection by Tab on disabled items

* add unit tests

* use item directly in function

* changelog

* remove boolean conversion
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

3 participants