-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 responsiveMode prop regression #17526
Fix responsiveMode prop regression #17526
Conversation
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 fe13c89:
|
Asset size changes
Baseline commit: 4301b994533a3c4e1d23316d2a975399dcf69898 (build) |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 881 | 814 | 5000 | |
BaseButton | mount | 883 | 888 | 5000 | |
Breadcrumb | mount | 2608 | 2574 | 1000 | |
ButtonNext | mount | 483 | 481 | 5000 | |
Checkbox | mount | 1469 | 1480 | 5000 | |
CheckboxBase | mount | 1258 | 1231 | 5000 | |
ChoiceGroup | mount | 4640 | 4675 | 5000 | |
ComboBox | mount | 999 | 958 | 1000 | |
CommandBar | mount | 10128 | 10055 | 1000 | |
ContextualMenu | mount | 6046 | 6058 | 1000 | |
DefaultButton | mount | 1114 | 1129 | 5000 | |
DetailsRow | mount | 3644 | 3650 | 5000 | |
DetailsRowFast | mount | 3722 | 3652 | 5000 | |
DetailsRowNoStyles | mount | 3552 | 3435 | 5000 | |
Dialog | mount | 2105 | 2142 | 1000 | |
DocumentCardTitle | mount | 139 | 145 | 1000 | |
Dropdown | mount | 3192 | 3526 | 5000 | |
FocusTrapZone | mount | 1801 | 1793 | 5000 | |
FocusZone | mount | 1757 | 1865 | 5000 | |
IconButton | mount | 1698 | 1688 | 5000 | |
Label | mount | 323 | 324 | 5000 | |
Layer | mount | 1745 | 1731 | 5000 | |
Link | mount | 453 | 445 | 5000 | |
MakeStyles | mount | 1797 | 1734 | 50000 | |
MenuButton | mount | 1439 | 1452 | 5000 | |
MessageBar | mount | 2015 | 1986 | 5000 | |
Nav | mount | 3230 | 3211 | 1000 | |
OverflowSet | mount | 1011 | 993 | 5000 | |
Panel | mount | 2052 | 2018 | 1000 | |
Persona | mount | 808 | 802 | 1000 | |
Pivot | mount | 1350 | 1379 | 1000 | |
PrimaryButton | mount | 1285 | 1242 | 5000 | |
Rating | mount | 7419 | 7412 | 5000 | |
SearchBox | mount | 1250 | 1287 | 5000 | |
Shimmer | mount | 2409 | 2430 | 5000 | |
Slider | mount | 1979 | 1902 | 5000 | |
SpinButton | mount | 4811 | 4828 | 5000 | |
Spinner | mount | 408 | 427 | 5000 | |
SplitButton | mount | 3033 | 3070 | 5000 | |
Stack | mount | 489 | 474 | 5000 | |
StackWithIntrinsicChildren | mount | 1564 | 1452 | 5000 | |
StackWithTextChildren | mount | 4348 | 4493 | 5000 | |
SwatchColorPicker | mount | 9944 | 10066 | 5000 | |
Tabs | mount | 1372 | 1353 | 1000 | |
TagPicker | mount | 2399 | 2422 | 5000 | |
TeachingBubble | mount | 11596 | 11776 | 5000 | |
Text | mount | 400 | 401 | 5000 | |
TextField | mount | 1358 | 1363 | 5000 | |
ThemeProvider | mount | 1173 | 1154 | 5000 | |
ThemeProvider | virtual-rerender | 590 | 590 | 5000 | |
ThemeProviderNext | mount | 6780 | 6879 | 5000 | |
Toggle | mount | 770 | 754 | 5000 | |
buttonNative | mount | 115 | 118 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
AttachmentMinimalPerf.default | 173 | 156 | 1.11:1 |
ListWith60ListItems.default | 662 | 607 | 1.09:1 |
RefMinimalPerf.default | 246 | 225 | 1.09:1 |
AccordionMinimalPerf.default | 155 | 148 | 1.05:1 |
InputMinimalPerf.default | 1272 | 1212 | 1.05:1 |
TextMinimalPerf.default | 331 | 314 | 1.05:1 |
ButtonMinimalPerf.default | 168 | 162 | 1.04:1 |
ChatMinimalPerf.default | 600 | 577 | 1.04:1 |
HeaderMinimalPerf.default | 379 | 366 | 1.04:1 |
DropdownManyItemsPerf.default | 656 | 638 | 1.03:1 |
RadioGroupMinimalPerf.default | 432 | 419 | 1.03:1 |
SliderMinimalPerf.default | 1560 | 1509 | 1.03:1 |
TextAreaMinimalPerf.default | 468 | 455 | 1.03:1 |
TooltipMinimalPerf.default | 978 | 945 | 1.03:1 |
AttachmentSlotsPerf.default | 1127 | 1102 | 1.02:1 |
DatepickerMinimalPerf.default | 5336 | 5211 | 1.02:1 |
ImageMinimalPerf.default | 362 | 354 | 1.02:1 |
ProviderMergeThemesPerf.default | 1680 | 1641 | 1.02:1 |
SegmentMinimalPerf.default | 334 | 329 | 1.02:1 |
SplitButtonMinimalPerf.default | 3677 | 3607 | 1.02:1 |
TableManyItemsPerf.default | 1863 | 1832 | 1.02:1 |
TreeMinimalPerf.default | 759 | 746 | 1.02:1 |
TreeWith60ListItems.default | 171 | 168 | 1.02:1 |
AlertMinimalPerf.default | 262 | 260 | 1.01:1 |
ButtonOverridesMissPerf.default | 1639 | 1627 | 1.01:1 |
CheckboxMinimalPerf.default | 2682 | 2646 | 1.01:1 |
DividerMinimalPerf.default | 343 | 341 | 1.01:1 |
GridMinimalPerf.default | 333 | 329 | 1.01:1 |
ProviderMinimalPerf.default | 970 | 961 | 1.01:1 |
SkeletonMinimalPerf.default | 340 | 337 | 1.01:1 |
IconMinimalPerf.default | 596 | 592 | 1.01:1 |
TableMinimalPerf.default | 397 | 393 | 1.01:1 |
CustomToolbarPrototype.default | 3806 | 3753 | 1.01:1 |
ToolbarMinimalPerf.default | 918 | 910 | 1.01:1 |
CardMinimalPerf.default | 525 | 527 | 1:1 |
ChatWithPopoverPerf.default | 363 | 364 | 1:1 |
DialogMinimalPerf.default | 721 | 718 | 1:1 |
EmbedMinimalPerf.default | 4011 | 4000 | 1:1 |
HeaderSlotsPerf.default | 756 | 758 | 1:1 |
LabelMinimalPerf.default | 384 | 384 | 1:1 |
ListNestedPerf.default | 526 | 525 | 1:1 |
LoaderMinimalPerf.default | 679 | 680 | 1:1 |
MenuButtonMinimalPerf.default | 1490 | 1488 | 1:1 |
StatusMinimalPerf.default | 653 | 652 | 1:1 |
DropdownMinimalPerf.default | 2998 | 3032 | 0.99:1 |
ItemLayoutMinimalPerf.default | 1231 | 1238 | 0.99:1 |
MenuMinimalPerf.default | 826 | 833 | 0.99:1 |
LayoutMinimalPerf.default | 360 | 366 | 0.98:1 |
ListMinimalPerf.default | 477 | 488 | 0.98:1 |
PopupMinimalPerf.default | 545 | 554 | 0.98:1 |
ReactionMinimalPerf.default | 354 | 362 | 0.98:1 |
AnimationMinimalPerf.default | 394 | 407 | 0.97:1 |
FormMinimalPerf.default | 399 | 410 | 0.97:1 |
BoxMinimalPerf.default | 335 | 348 | 0.96:1 |
CarouselMinimalPerf.default | 438 | 455 | 0.96:1 |
RosterPerf.default | 1121 | 1165 | 0.96:1 |
PortalMinimalPerf.default | 152 | 159 | 0.96:1 |
AvatarMinimalPerf.default | 186 | 196 | 0.95:1 |
ButtonSlotsPerf.default | 535 | 561 | 0.95:1 |
ListCommonPerf.default | 591 | 625 | 0.95:1 |
ChatDuplicateMessagesPerf.default | 267 | 285 | 0.94:1 |
FlexMinimalPerf.default | 280 | 299 | 0.94:1 |
VideoMinimalPerf.default | 566 | 604 | 0.94:1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI! |
…ix/dropdown-responsive-mode
@ecraig12345 Any updates? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates and so sorry for the delayed review! I think this approach is better, but as the useResponsiveMode
code is now, it's only going to use the override if it was passed in on initial render (and won't respect updates).
Here's what I would suggest:
- Revert the changes to
lastResponsiveMode
and instead, always return the override param's current value if provided (last line) - Call
onResize
if the param changes to undefined (in addition to on initial mount) - (minor) Change the name of the param to
overrideResponsiveMode
to be more explicit
Possible code for these suggestions is below (since I wrote it locally while trying to figure out what alternative approach to recommend).
/**
* Hook to get the current responsive mode (window size category).
* @param elementRef - Use this element's parent window when determining the responsive mode.
* @param overrideResponsiveMode - Override the responsive mode. If this param is present, it's always returned.
*/
export const useResponsiveMode = (
elementRef: React.RefObject<HTMLElement | null>,
overrideResponsiveMode?: ResponsiveMode,
) => {
const [lastResponsiveMode, setLastResponsiveMode] = React.useState<ResponsiveMode>(getInitialResponsiveMode());
const onResize = React.useCallback(() => {
if (overrideResponsiveMode === undefined) {
const newResponsiveMode = getResponsiveMode(getWindow(elementRef.current));
// Setting the same value should not cause a re-render.
if (lastResponsiveMode !== newResponsiveMode) {
setLastResponsiveMode(newResponsiveMode);
}
}
}, [elementRef, lastResponsiveMode, overrideResponsiveMode]);
const win = useWindow();
useOnEvent(win, 'resize', onResize);
// Call resize function initially on mount, or if the override changes from defined to undefined
// (the effect will run on all override changes, but onResize will only be called if it changed to undefined)
React.useEffect(() => {
if (overrideResponsiveMode === undefined) {
onResize();
}
// eslint-disable-next-line react-hooks/exhaustive-deps -- only meant to run on mount or when override changes
}, [overrideResponsiveMode]);
return overrideResponsiveMode ?? lastResponsiveMode;
};
This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI! |
Any updates? |
Thank you for your comment. I've refactored the hook. /**
* Hook to get the current responsive mode (window size category).
* @param elementRef - Use this element's parent window when determining the responsive mode.
* @param overrideResponsiveMode - Override the responsive mode. If this param is present, it's always returned.
*/
export const useResponsiveMode = (
elementRef: React.RefObject<HTMLElement | null>,
overrideResponsiveMode?: ResponsiveMode,
) => {
const [lastResponsiveMode, setLastResponsiveMode] = React.useState<ResponsiveMode>(getInitialResponsiveMode());
const onResize = React.useCallback(() => {
const newResponsiveMode = getResponsiveMode(getWindow(elementRef.current));
// Setting the same value should not cause a re-render.
if (lastResponsiveMode !== newResponsiveMode) {
setLastResponsiveMode(newResponsiveMode);
}
}, [elementRef, lastResponsiveMode]);
const win = useWindow();
useOnEvent(win, 'resize', onResize);
// Call resize function initially on mount, or if the override changes from defined to undefined
// (the effect will run on all override changes, but onResize will only be called if it changed to undefined)
React.useEffect(() => {
if (overrideResponsiveMode === undefined) {
onResize();
}
// eslint-disable-next-line react-hooks/exhaustive-deps -- only meant to run on mount or when override changes
}, [overrideResponsiveMode]);
return overrideResponsiveMode ?? lastResponsiveMode;
}; |
🎉 Handy links: |
Pull request checklist
$ yarn change
Description of changes
responsiveMode
prop regression in Fluent UI v8. PoC: Dropdown responsiveMode prop doesn't work in version 8 #17288 (comment).