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(bindings): add useUnhandledProps hook #12371

Merged
merged 6 commits into from
Mar 20, 2020

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Mar 20, 2020

Pull request checklist

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

Description of changes

This PR adds useUnhandledProps() hook to replace getUnhandledProps() function in functional components. Functionality of useUnhandledProps() will be changed in compose() PR.

Microsoft Reviewers: Open in CodeFlow

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Mar 20, 2020

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 746 726
BaseButton (experiments) 842 856
DefaultButton 963 952
DefaultButton (experiments) 1717 1723
DetailsRow 3040 3147
DetailsRow (fast icons) 3136 3114
DetailsRow without styles 2934 2948
DocumentCardTitle with truncation 1525 1550
MenuButton 1289 1263
MenuButton (experiments) 3161 3113
PrimaryButton 1093 1097
PrimaryButton (experiments) 1782 1746
SplitButton 2736 2720
SplitButton (experiments) 6463 6362
Stack 428 409
Stack with Intrinsic children 985 999
Stack with Text children 3634 3614
Text 327 326
Toggle 784 782
Toggle (experiments) 1993 1995
button 55 55

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.51 0.46 1.11:1 2000 1016
🦄 Button.Fluent 0.09 0.17 0.53:1 5000 456
🔧 Checkbox.Fluent 0.72 0.36 2:1 1000 724
🔧 Dialog.Fluent 0.38 0.19 2:1 5000 1913
🔧 Dropdown.Fluent 3.57 0.45 7.93:1 1000 3574
🔧 Icon.Fluent 0.16 0.05 3.2:1 5000 818
🦄 Image.Fluent 0.06 0.1 0.6:1 5000 316
🔧 Slider.Fluent 1.55 0.4 3.88:1 1000 1548
🔧 Text.Fluent 0.07 0.02 3.5:1 5000 349
🦄 Tooltip.Fluent 0.12 15.84 0.01:1 5000 580

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AnimationMinimalPerf.default 601 559 1.08:1
ImageMinimalPerf.default 338 316 1.07:1
DropdownMinimalPerf.default 3737 3522 1.06:1
TableMinimalPerf.default 649 614 1.06:1
Button.Fluent 456 433 1.05:1
ProviderMergeThemesPerf.default 1409 1349 1.04:1
Checkbox.Fluent 724 694 1.04:1
Icon.Fluent 818 784 1.04:1
AttachmentMinimalPerf.default 933 906 1.03:1
LabelMinimalPerf.default 373 363 1.03:1
ListMinimalPerf.default 431 419 1.03:1
TextAreaMinimalPerf.default 3188 3083 1.03:1
ToolbarMinimalPerf.default 1075 1043 1.03:1
ButtonMinimalPerf.default 146 143 1.02:1
MenuButtonMinimalPerf.default 1552 1525 1.02:1
ButtonSlotsPerf.default 601 598 1.01:1
CarouselMinimalPerf.default 2052 2026 1.01:1
DividerMinimalPerf.default 949 943 1.01:1
InputMinimalPerf.default 1071 1063 1.01:1
ListNestedPerf.default 904 896 1.01:1
SegmentMinimalPerf.default 1066 1056 1.01:1
SliderMinimalPerf.default 1581 1565 1.01:1
CustomToolbarPrototype.default 3747 3707 1.01:1
TreeWith60ListItems.default 231 228 1.01:1
Dropdown.Fluent 3574 3541 1.01:1
Text.Fluent 349 344 1.01:1
BoxMinimalPerf.default 339 338 1:1
ChatDuplicateMessagesPerf.default 416 415 1:1
EmbedMinimalPerf.default 5437 5418 1:1
GridMinimalPerf.default 828 831 1:1
HeaderMinimalPerf.default 524 525 1:1
HierarchicalTreeMinimalPerf.default 996 995 1:1
ItemLayoutMinimalPerf.default 1997 2006 1:1
ListCommonPerf.default 1031 1035 1:1
ListWith60ListItems.default 1190 1189 1:1
MenuMinimalPerf.default 1984 1981 1:1
PopupMinimalPerf.default 247 248 1:1
PortalMinimalPerf.default 275 275 1:1
ProviderMinimalPerf.default 636 633 1:1
RadioGroupMinimalPerf.default 538 540 1:1
ReactionMinimalPerf.default 2481 2483 1:1
SplitButtonMinimalPerf.default 12417 12405 1:1
StatusMinimalPerf.default 585 586 1:1
TooltipMinimalPerf.default 839 839 1:1
Tooltip.Fluent 580 579 1:1
AccordionMinimalPerf.default 231 234 0.99:1
AttachmentSlotsPerf.default 3562 3596 0.99:1
AvatarMinimalPerf.default 543 547 0.99:1
DialogMinimalPerf.default 1895 1912 0.99:1
HeaderSlotsPerf.default 1593 1613 0.99:1
RefMinimalPerf.default 195 197 0.99:1
TextMinimalPerf.default 372 374 0.99:1
TreeMinimalPerf.default 1145 1151 0.99:1
Dialog.Fluent 1913 1936 0.99:1
CheckboxMinimalPerf.default 3223 3291 0.98:1
FormMinimalPerf.default 904 922 0.98:1
IconMinimalPerf.default 390 400 0.98:1
LayoutMinimalPerf.default 621 631 0.98:1
LoaderMinimalPerf.default 1056 1082 0.98:1
Image.Fluent 316 324 0.98:1
Slider.Fluent 1548 1573 0.98:1
ChatWithPopoverPerf.default 597 616 0.97:1
DropdownManyItemsPerf.default 1436 1486 0.97:1
FlexMinimalPerf.default 243 251 0.97:1
Avatar.Fluent 1016 1043 0.97:1
VideoMinimalPerf.default 843 876 0.96:1
AlertMinimalPerf.default 555 600 0.93:1
ChatMinimalPerf.default 517 570 0.91:1

@size-auditor
Copy link

size-auditor bot commented Mar 20, 2020

Asset size changes

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

Baseline commit: 13e558c9ff85536a1ce1b06d4816b78fab2ec7be (build)

* @param props - A ReactElement props object
* @returns A shallow copy of the prop object
*/
function useUnhandledProps<P extends Record<string, any>>(handledProps: (keyof P)[], props: P): Partial<P> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add example in the react-bindings README? Or do you consider this as an internal hook?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 👍

@@ -15,6 +15,7 @@ export { default as unstable_useDispatchEffect } from './hooks/useDispatchEffect
export { default as useIsomorphicLayoutEffect } from './hooks/useIsomorphicLayoutEffect';
export { default as useStateManager } from './hooks/useStateManager';
export { default as useStyles } from './hooks/useStyles';
export { default as useUnhandledProps } from './hooks/useUnhandledProps';
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are exporting it, I believe it deserves changelog entry

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, added

@@ -231,7 +231,7 @@ const Animation: React.FC<AnimationProps> & {
const { animationDuration, animationDelay } = animationStyles.root;
const timeoutResult = timeout || calculateAnimationTimeout(animationDuration, animationDelay) || 0;

const unhandledProps = getUnhandledProps(Animation.handledProps, props);
const unhandledProps = useUnhandledProps(Animation.handledProps, props);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the generic typings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, they will be inferred as it was previously for getUnhandledProps

@layershifter layershifter merged commit e2a5a85 into master Mar 20, 2020
@layershifter layershifter deleted the feat/add-use-unhandled-props branch March 20, 2020 14:02
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

4 participants