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

Add new hooks used in migrating Fabric components to functional components #12629

Merged
merged 22 commits into from
Apr 21, 2020

Conversation

MLoughry
Copy link
Contributor

@MLoughry MLoughry commented Apr 9, 2020

Pull request checklist

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

Description of changes

The hook changes/additions are plucked from my branches migrating a couple components to functional components (https://github.com/MLoughry/office-ui-fabric-react/tree/functional/fabric, https://github.com/MLoughry/office-ui-fabric-react/tree/functional/checkbox). While those changes may have to wait for a new major version, these changes are non-breaking and will allow me to use these hooks across further migrations while we figure out how/when to merge the migrations.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@size-auditor
Copy link

size-auditor bot commented Apr 9, 2020

Asset size changes

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

Baseline commit: 7fc234896718e61af6113ddab56f3d6d7ee20154 (build)

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Apr 9, 2020

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 716 674 5000
Checkbox 1565 1577 5000
CheckboxBase 1298 1246 5000
ChoiceGroup 4627 4558 5000
ComboBox 855 918 1000
CommandBar 6864 6830 1000
DefaultButton 906 923 5000
DetailsRow 3012 2979 5000
DetailsRow (fast icons) 2999 3044 5000
DetailsRow without styles 2873 2789 5000
Dialog 1285 1285 1000
DocumentCardTitle with truncation 1485 1474 1000
Dropdown 2624 2801 5000
FocusZone 1395 1373 5000
IconButton 1501 1600 5000
Label 402 430 5000
Link 388 398 5000
MenuButton 1219 1204 5000
Nav 2770 2769 1000
Panel 1267 1263 1000
Persona 704 733 1000
Pivot 1139 1122 1000
PrimaryButton 1059 1060 5000
SearchBox 1260 1283 5000
Slider 1627 1625 5000
Spinner 351 355 5000
SplitButton 2694 2824 5000
Stack 430 412 5000
Stack with Intrinsic children 1010 1020 5000
Stack with Text children 3628 3686 5000
TagPicker 2501 2508 5000
Text 325 350 5000
TextField 1613 1594 5000
Toggle 793 781 5000
button 61 57 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.44 0.44 1:1 2000 874
🦄 Button.Fluent 0.09 0.17 0.53:1 5000 431
🔧 Checkbox.Fluent 0.58 0.36 1.61:1 1000 584
🔧 Dialog.Fluent 0.3 0.18 1.67:1 5000 1522
🔧 Dropdown.Fluent 3.05 0.41 7.44:1 1000 3051
🔧 Icon.Fluent 0.12 0.04 3:1 5000 605
🎯 Image.Fluent 0.07 0.09 0.78:1 5000 327
🔧 Slider.Fluent 1.31 0.37 3.54:1 1000 1308
🔧 Text.Fluent 0.07 0.02 3.5:1 5000 325
🦄 Tooltip.Fluent 0.08 17.28 0:1 5000 405

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 139 129 1.08:1
LoaderMinimalPerf.default 732 684 1.07:1
ImageMinimalPerf.default 353 333 1.06:1
Dropdown.Fluent 3051 2892 1.05:1
CarouselMinimalPerf.default 568 546 1.04:1
ItemLayoutMinimalPerf.default 1544 1486 1.04:1
VideoMinimalPerf.default 713 687 1.04:1
Slider.Fluent 1308 1263 1.04:1
ReactionMinimalPerf.default 1700 1653 1.03:1
TreeWith60ListItems.default 203 197 1.03:1
ChatMinimalPerf.default 588 574 1.02:1
CheckboxMinimalPerf.default 2706 2665 1.02:1
HeaderSlotsPerf.default 1326 1296 1.02:1
InputMinimalPerf.default 953 934 1.02:1
MenuMinimalPerf.default 1672 1635 1.02:1
Avatar.Fluent 874 853 1.02:1
Text.Fluent 325 319 1.02:1
AlertMinimalPerf.default 477 474 1.01:1
CardMinimalPerf.default 524 517 1.01:1
LabelMinimalPerf.default 369 365 1.01:1
LayoutMinimalPerf.default 495 492 1.01:1
ListCommonPerf.default 898 886 1.01:1
ProviderMergeThemesPerf.default 1489 1470 1.01:1
RefMinimalPerf.default 185 184 1.01:1
SliderMinimalPerf.default 1257 1250 1.01:1
SplitButtonMinimalPerf.default 3359 3314 1.01:1
TextMinimalPerf.default 321 317 1.01:1
TextAreaMinimalPerf.default 2479 2458 1.01:1
AccordionMinimalPerf.default 175 175 1:1
ButtonMinimalPerf.default 144 144 1:1
DividerMinimalPerf.default 664 666 1:1
DropdownMinimalPerf.default 3035 3021 1:1
ListNestedPerf.default 809 805 1:1
ListWith60ListItems.default 1057 1057 1:1
MenuButtonMinimalPerf.default 1376 1372 1:1
ProviderMinimalPerf.default 624 624 1:1
RadioGroupMinimalPerf.default 485 487 1:1
SegmentMinimalPerf.default 811 811 1:1
ToolbarMinimalPerf.default 876 878 1:1
Dialog.Fluent 1522 1515 1:1
AttachmentSlotsPerf.default 1010 1022 0.99:1
ButtonSlotsPerf.default 548 554 0.99:1
ChatWithPopoverPerf.default 552 559 0.99:1
DialogMinimalPerf.default 1565 1574 0.99:1
EmbedMinimalPerf.default 3926 3947 0.99:1
FormMinimalPerf.default 652 658 0.99:1
HierarchicalTreeMinimalPerf.default 878 890 0.99:1
ListMinimalPerf.default 431 435 0.99:1
IconMinimalPerf.default 603 608 0.99:1
TableMinimalPerf.default 481 486 0.99:1
TooltipMinimalPerf.default 633 639 0.99:1
Button.Fluent 431 436 0.99:1
Checkbox.Fluent 584 589 0.99:1
ChatDuplicateMessagesPerf.default 382 388 0.98:1
FlexMinimalPerf.default 263 269 0.98:1
PopupMinimalPerf.default 237 243 0.98:1
PortalMinimalPerf.default 266 271 0.98:1
CustomToolbarPrototype.default 3294 3373 0.98:1
TreeMinimalPerf.default 981 1003 0.98:1
Image.Fluent 327 333 0.98:1
AvatarMinimalPerf.default 471 484 0.97:1
GridMinimalPerf.default 604 621 0.97:1
AnimationMinimalPerf.default 576 602 0.96:1
StatusMinimalPerf.default 603 630 0.96:1
Tooltip.Fluent 405 420 0.96:1
HeaderMinimalPerf.default 424 445 0.95:1
Icon.Fluent 605 640 0.95:1
BoxMinimalPerf.default 287 311 0.92:1
DropdownManyItemsPerf.default 1216 1334 0.91:1

@MLoughry
Copy link
Contributor Author

MLoughry commented Apr 9, 2020

Not sure why the Screener baseline is missing an image

Copy link
Member

@dzearing dzearing left a comment

Choose a reason for hiding this comment

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

Overall looks great, just the one question about useControllableState

@MLoughry
Copy link
Contributor Author

Ping. @dzearing, is this ok to merge?

Copy link
Member

@dzearing dzearing left a comment

Choose a reason for hiding this comment

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

Minor feedback

Copy link
Member

@dzearing dzearing left a comment

Choose a reason for hiding this comment

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

minor feedback, if the value doesn't change, don't call onChange.

@MLoughry
Copy link
Contributor Author

Per our Teams discussion, preventing the call to onChange if the value doesn't change would require adding the current state value to the dependencies on the setValueOrCallOnChange callback, which would very likely lead to more rerenders than would be saved in this corner case.

@dzearing dzearing merged commit b3651dd into microsoft:master Apr 21, 2020
@msft-github-bot
Copy link
Contributor

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

Handy links:

DuanShaolong pushed a commit to DuanShaolong/fluentui that referenced this pull request Apr 27, 2020
…nents (microsoft#12629)

* Add new hooks used in migrating Fabric components to functional components

* Change files

* Cherry pick revision from MLoughry:functional/checkbox

* Fix lint error

* Address comments from dzearing

* Fix launch.json for debugging tests

* Use common onChange call signature

* Fix useControllableValue types in case with no onChange handler

* Export ChangeCallback type

* Persist the controlled vs uncontrolled state

* Update packages/react-hooks/src/useMergedRefs.ts

Co-Authored-By: Elizabeth Craig <[email protected]>

* Update README

* Allow for undefined events in the onChange callback

* Fix error in isControlled value

* Delete test for deprecated behavior

* Update packages/react-hooks/README.md

Co-Authored-By: Elizabeth Craig <[email protected]>

* Update packages/react-hooks/README.md

Co-Authored-By: Elizabeth Craig <[email protected]>

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

Successfully merging this pull request may close these issues.

5 participants