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

Fix BasePicker's onDismiss behavior from changing if getTextFromItem provided #14302

Merged
merged 3 commits into from
Aug 12, 2020

Conversation

17jlee2
Copy link
Contributor

@17jlee2 17jlee2 commented Aug 3, 2020

Pull request checklist

Description of changes

As suggested in #10892, this changes the return type of the BasePicker's onDismiss prop to optionally return a boolean. If it returns false, then the first suggested item is not selected when the suggestions callout is dismissed. If it returns anything else, then the default behavior of it selecting the first suggestion is preserved.

Focus areas to test

The BasePicker and its derivatives (like TagPicker)

@ghost
Copy link

ghost commented Aug 3, 2020

CLA assistant check
All CLA requirements met.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 3, 2020

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 d0a75b6:

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
Picker calling onChange incorrectly Issue #10892

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Aug 3, 2020

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 906 950 5000
ButtonNext mount 598 597 5000
Checkbox mount 1618 1641 5000
CheckboxBase mount 1395 1359 5000
CheckboxNext mount 1732 1705 5000
ChoiceGroup mount 5168 5292 5000
ComboBox mount 920 934 1000
CommandBar mount 7667 7478 1000
ContextualMenu mount 12664 12721 1000
DefaultButton mount 1136 1115 5000
DetailsRow mount 3616 3661 5000
DetailsRowFast mount 3604 3757 5000
DetailsRowNoStyles mount 3456 3466 5000
Dialog mount 1518 1529 1000
DocumentCardTitle mount 1817 1784 1000
Dropdown mount 2682 2683 5000
FocusZone mount 1785 1811 5000
IconButton mount 1815 1821 5000
Label mount 329 334 5000
Link mount 474 449 5000
LinkNext mount 488 479 5000
MenuButton mount 1528 1532 5000
Nav mount 3645 3288 1000
Panel mount 1469 1467 1000
Persona mount 853 856 1000
Pivot mount 1443 1435 1000
PivotNext mount 1439 1424 1000
PrimaryButton mount 1329 1334 5000
SearchBox mount 1331 1391 5000
SearchBoxNext mount 1375 1405 5000
Slider mount 1574 1547 5000
SliderNext mount 1997 1988 5000
SpinButton mount 5100 5158 5000
SpinButtonNext mount 5197 5160 5000
Spinner mount 406 423 5000
SplitButton mount 3208 3162 5000
Stack mount 534 560 5000
StackWithIntrinsicChildren mount 1944 1996 5000
StackWithTextChildren mount 5343 5241 5000
TagPicker mount 2751 2788 5000
Text mount 446 465 5000
TextField mount 1424 1408 5000
ThemeProvider mount 2746 2757 5000
ThemeProvider virtual-rerender 446 440 5000
Toggle mount 859 894 5000
ToggleNext mount 870 850 5000
button mount 116 103 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.46 0.49 0.94:1 2000 917
🦄 Button.Fluent 0.12 0.2 0.6:1 5000 590
🔧 Checkbox.Fluent 0.65 0.37 1.76:1 1000 647
🎯 Dialog.Fluent 0.16 0.22 0.73:1 5000 796
🔧 Dropdown.Fluent 2.99 0.47 6.36:1 1000 2987
🔧 Icon.Fluent 0.14 0.05 2.8:1 5000 722
🦄 Image.Fluent 0.08 0.12 0.67:1 5000 400
🔧 Slider.Fluent 1.54 0.38 4.05:1 1000 1542
🔧 Text.Fluent 0.07 0.02 3.5:1 5000 346
🦄 Tooltip.Fluent 0.11 15.45 0.01:1 5000 544

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 179 155 1.15:1
GridMinimalPerf.default 381 332 1.15:1
ButtonMinimalPerf.default 189 169 1.12:1
FormMinimalPerf.default 476 432 1.1:1
BoxMinimalPerf.default 361 336 1.07:1
ChatDuplicateMessagesPerf.default 453 423 1.07:1
SkeletonMinimalPerf.default 443 414 1.07:1
AnimationMinimalPerf.default 411 388 1.06:1
DropdownManyItemsPerf.default 841 792 1.06:1
VideoMinimalPerf.default 664 629 1.06:1
AttachmentSlotsPerf.default 1194 1132 1.05:1
AvatarMinimalPerf.default 523 498 1.05:1
HeaderSlotsPerf.default 870 825 1.05:1
ImageMinimalPerf.default 386 366 1.05:1
StatusMinimalPerf.default 737 699 1.05:1
ChatMinimalPerf.default 636 613 1.04:1
ChatWithPopoverPerf.default 501 482 1.04:1
DialogMinimalPerf.default 817 788 1.04:1
DividerMinimalPerf.default 372 357 1.04:1
FlexMinimalPerf.default 302 290 1.04:1
ItemLayoutMinimalPerf.default 1344 1289 1.04:1
ProviderMinimalPerf.default 919 881 1.04:1
ToolbarMinimalPerf.default 1018 981 1.04:1
Avatar.Fluent 917 885 1.04:1
Button.Fluent 590 567 1.04:1
HeaderMinimalPerf.default 376 366 1.03:1
ListNestedPerf.default 931 905 1.03:1
ListWith60ListItems.default 1130 1096 1.03:1
SegmentMinimalPerf.default 366 354 1.03:1
TextMinimalPerf.default 357 345 1.03:1
TooltipMinimalPerf.default 807 781 1.03:1
EmbedMinimalPerf.default 2023 1974 1.02:1
InputMinimalPerf.default 1322 1297 1.02:1
ListMinimalPerf.default 503 491 1.02:1
LoaderMinimalPerf.default 759 747 1.02:1
ProviderMergeThemesPerf.default 1854 1814 1.02:1
ReactionMinimalPerf.default 411 401 1.02:1
RefMinimalPerf.default 205 200 1.02:1
IconMinimalPerf.default 703 690 1.02:1
TableManyItemsPerf.default 2371 2328 1.02:1
TextAreaMinimalPerf.default 513 501 1.02:1
CustomToolbarPrototype.default 3712 3644 1.02:1
TreeMinimalPerf.default 904 887 1.02:1
Dropdown.Fluent 2987 2919 1.02:1
Tooltip.Fluent 544 531 1.02:1
AlertMinimalPerf.default 327 325 1.01:1
HierarchicalTreeMinimalPerf.default 435 432 1.01:1
ListCommonPerf.default 1000 992 1.01:1
MenuButtonMinimalPerf.default 1613 1591 1.01:1
SplitButtonMinimalPerf.default 3939 3886 1.01:1
Image.Fluent 400 396 1.01:1
PopupMinimalPerf.default 664 662 1:1
Dialog.Fluent 796 793 1:1
Text.Fluent 346 345 1:1
CardMinimalPerf.default 597 601 0.99:1
CheckboxMinimalPerf.default 2868 2890 0.99:1
DropdownMinimalPerf.default 2905 2920 0.99:1
RadioGroupMinimalPerf.default 453 456 0.99:1
AccordionMinimalPerf.default 156 160 0.98:1
ButtonSlotsPerf.default 608 619 0.98:1
SliderMinimalPerf.default 1559 1589 0.98:1
Checkbox.Fluent 647 659 0.98:1
Slider.Fluent 1542 1578 0.98:1
CarouselMinimalPerf.default 475 492 0.97:1
LabelMinimalPerf.default 411 424 0.97:1
TableMinimalPerf.default 409 422 0.97:1
TreeWith60ListItems.default 221 229 0.97:1
Icon.Fluent 722 747 0.97:1
MenuMinimalPerf.default 888 923 0.96:1
PortalMinimalPerf.default 122 127 0.96:1
LayoutMinimalPerf.default 399 418 0.95:1

@size-auditor
Copy link

size-auditor bot commented Aug 3, 2020

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react office-ui-fabric-react-Pickers 273.369 kB 273.39 kB ExceedsBaseline     21 bytes
office-ui-fabric-react fluentui-react-next-Pickers 273.369 kB 273.39 kB ExceedsBaseline     21 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: f2c29686e5fb0d71db3a1e02ee4a7f3fdc185d03 (build)

@17jlee2 17jlee2 changed the title Change BasePicker's onDismiss to optionally return a boolean Fix BasePicker's onDismiss behavior from changing if getTextFromItem provided Aug 3, 2020
@17jlee2
Copy link
Contributor Author

17jlee2 commented Aug 3, 2020

Asset size changes

⚠️ Insufficient baseline data to detect size changes

Unable to find bundle size details for Baseline commit: bc90b07

Possible causes

  • The baseline build bc90b07 is broken
  • The Size Auditor run for the baseline build bc90b07 was not triggered

Recommendations

  • Please merge your branch for this Pull request with the latest master build and commit your changes once again

@xugao @joschect Hey! I'm not very familiar with the workflow here. Is the recommendation saying to update my fork with the main repo, to create another PR, or something else entirely?

@xugao
Copy link
Contributor

xugao commented Aug 10, 2020

if you pull master again , the size auditor error should go away

Recommendations
Please merge your branch for this Pull request with the latest master build and commit your changes once again

@17jlee2
Copy link
Contributor Author

17jlee2 commented Aug 10, 2020

if you pull master again , the size auditor error should go away

Recommendations
Please merge your branch for this Pull request with the latest master build and commit your changes once again

I think I understand, thanks!

@17jlee2 17jlee2 requested a review from xugao August 10, 2020 22:39
Copy link
Contributor

@joschect joschect left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for making this pr. I have a few comments but other than that it should be ready!

@xugao xugao merged commit 11f792c into microsoft:master Aug 12, 2020
@msft-github-bot
Copy link
Contributor

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

levithomason pushed a commit to levithomason/fluentui that referenced this pull request Aug 24, 2020
…provided (microsoft#14302)

* BasePicker's onDismiss can now return a boolean to decide if an item is selected

* fixed a typo

Co-authored-by: James <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Picker getTextFromItem changes dismiss behavior
4 participants