Fix tooltip styling broken by a change to makeStyles #18966
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 1a2a3ae:
|
📊 Bundle size report🤖 This report was generated against a7f937bb031dbbfd2ab9c00d8d5ba31087f2a230 |
Asset size changes
Baseline commit: a7f937bb031dbbfd2ab9c00d8d5ba31087f2a230 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 796 | 809 | 5000 | |
| BaseButton | mount | 869 | 875 | 5000 | |
| Breadcrumb | mount | 2603 | 2594 | 1000 | |
| ButtonNext | mount | 534 | 519 | 5000 | |
| Checkbox | mount | 1517 | 1501 | 5000 | |
| CheckboxBase | mount | 1247 | 1260 | 5000 | |
| ChoiceGroup | mount | 4637 | 4650 | 5000 | |
| ComboBox | mount | 1022 | 949 | 1000 | |
| CommandBar | mount | 9987 | 9940 | 1000 | |
| ContextualMenu | mount | 6129 | 6168 | 1000 | |
| DefaultButton | mount | 1091 | 1081 | 5000 | |
| DetailsRow | mount | 3625 | 3616 | 5000 | |
| DetailsRowFast | mount | 3661 | 3610 | 5000 | |
| DetailsRowNoStyles | mount | 3442 | 3466 | 5000 | |
| Dialog | mount | 2126 | 2110 | 1000 | |
| DocumentCardTitle | mount | 139 | 139 | 1000 | |
| Dropdown | mount | 3185 | 3132 | 5000 | |
| FluentProviderNext | mount | 7254 | 7146 | 5000 | |
| FocusTrapZone | mount | 1762 | 1742 | 5000 | |
| FocusZone | mount | 1794 | 1759 | 5000 | |
| IconButton | mount | 1683 | 1680 | 5000 | |
| Label | mount | 329 | 334 | 5000 | |
| Layer | mount | 1738 | 1779 | 5000 | |
| Link | mount | 458 | 443 | 5000 | |
| MakeStyles | mount | 1789 | 1794 | 50000 | |
| MenuButton | mount | 1438 | 1446 | 5000 | |
| MessageBar | mount | 2008 | 1985 | 5000 | |
| Nav | mount | 3192 | 3188 | 1000 | |
| OverflowSet | mount | 1014 | 1051 | 5000 | |
| Panel | mount | 2089 | 2006 | 1000 | |
| Persona | mount | 809 | 813 | 1000 | |
| Pivot | mount | 1380 | 1380 | 1000 | |
| PrimaryButton | mount | 1240 | 1242 | 5000 | |
| Rating | mount | 7531 | 7442 | 5000 | |
| SearchBox | mount | 1311 | 1275 | 5000 | |
| Shimmer | mount | 2430 | 2458 | 5000 | |
| Slider | mount | 1891 | 1892 | 5000 | |
| SpinButton | mount | 4876 | 4826 | 5000 | |
| Spinner | mount | 417 | 426 | 5000 | |
| SplitButton | mount | 3069 | 3048 | 5000 | |
| Stack | mount | 472 | 479 | 5000 | |
| StackWithIntrinsicChildren | mount | 1498 | 1491 | 5000 | |
| StackWithTextChildren | mount | 4349 | 4374 | 5000 | |
| SwatchColorPicker | mount | 9877 | 9874 | 5000 | |
| Tabs | mount | 1365 | 1358 | 1000 | |
| TagPicker | mount | 2369 | 2322 | 5000 | |
| TeachingBubble | mount | 11621 | 11677 | 5000 | |
| Text | mount | 409 | 399 | 5000 | |
| TextField | mount | 1363 | 1325 | 5000 | |
| ThemeProvider | mount | 1152 | 1135 | 5000 | |
| ThemeProvider | virtual-rerender | 597 | 611 | 5000 | |
| Toggle | mount | 792 | 779 | 5000 | |
| buttonNative | mount | 116 | 123 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 159 | 139 | 1.14:1 |
| ListWith60ListItems.default | 660 | 595 | 1.11:1 |
| ChatWithPopoverPerf.default | 358 | 336 | 1.07:1 |
| FlexMinimalPerf.default | 277 | 259 | 1.07:1 |
| GridMinimalPerf.default | 335 | 313 | 1.07:1 |
| TextMinimalPerf.default | 349 | 326 | 1.07:1 |
| AccordionMinimalPerf.default | 150 | 143 | 1.05:1 |
| ChatMinimalPerf.default | 631 | 600 | 1.05:1 |
| TreeWith60ListItems.default | 173 | 165 | 1.05:1 |
| AttachmentSlotsPerf.default | 1076 | 1037 | 1.04:1 |
| ButtonMinimalPerf.default | 159 | 153 | 1.04:1 |
| PopupMinimalPerf.default | 589 | 567 | 1.04:1 |
| PortalMinimalPerf.default | 177 | 171 | 1.04:1 |
| ReactionMinimalPerf.default | 365 | 350 | 1.04:1 |
| VideoMinimalPerf.default | 611 | 590 | 1.04:1 |
| LayoutMinimalPerf.default | 350 | 341 | 1.03:1 |
| ListCommonPerf.default | 609 | 592 | 1.03:1 |
| ListMinimalPerf.default | 493 | 477 | 1.03:1 |
| RefMinimalPerf.default | 242 | 235 | 1.03:1 |
| SkeletonMinimalPerf.default | 346 | 336 | 1.03:1 |
| IconMinimalPerf.default | 608 | 593 | 1.03:1 |
| TreeMinimalPerf.default | 795 | 771 | 1.03:1 |
| AlertMinimalPerf.default | 267 | 262 | 1.02:1 |
| AvatarMinimalPerf.default | 190 | 186 | 1.02:1 |
| ButtonOverridesMissPerf.default | 1654 | 1629 | 1.02:1 |
| ChatDuplicateMessagesPerf.default | 278 | 273 | 1.02:1 |
| DialogMinimalPerf.default | 739 | 724 | 1.02:1 |
| FormMinimalPerf.default | 382 | 376 | 1.02:1 |
| HeaderMinimalPerf.default | 348 | 341 | 1.02:1 |
| ToolbarMinimalPerf.default | 915 | 898 | 1.02:1 |
| BoxMinimalPerf.default | 343 | 338 | 1.01:1 |
| CardMinimalPerf.default | 539 | 533 | 1.01:1 |
| CheckboxMinimalPerf.default | 2645 | 2621 | 1.01:1 |
| DropdownMinimalPerf.default | 3057 | 3039 | 1.01:1 |
| HeaderSlotsPerf.default | 736 | 726 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1170 | 1153 | 1.01:1 |
| MenuMinimalPerf.default | 824 | 812 | 1.01:1 |
| RadioGroupMinimalPerf.default | 437 | 432 | 1.01:1 |
| SegmentMinimalPerf.default | 341 | 338 | 1.01:1 |
| SplitButtonMinimalPerf.default | 3680 | 3653 | 1.01:1 |
| TableManyItemsPerf.default | 1826 | 1811 | 1.01:1 |
| CustomToolbarPrototype.default | 3752 | 3727 | 1.01:1 |
| TooltipMinimalPerf.default | 987 | 976 | 1.01:1 |
| CarouselMinimalPerf.default | 436 | 437 | 1:1 |
| DatepickerMinimalPerf.default | 5252 | 5273 | 1:1 |
| EmbedMinimalPerf.default | 3972 | 3969 | 1:1 |
| ImageMinimalPerf.default | 350 | 349 | 1:1 |
| LoaderMinimalPerf.default | 672 | 675 | 1:1 |
| MenuButtonMinimalPerf.default | 1596 | 1591 | 1:1 |
| ProviderMergeThemesPerf.default | 1633 | 1632 | 1:1 |
| SliderMinimalPerf.default | 1526 | 1531 | 1:1 |
| StatusMinimalPerf.default | 646 | 645 | 1:1 |
| InputMinimalPerf.default | 1217 | 1232 | 0.99:1 |
| LabelMinimalPerf.default | 356 | 359 | 0.99:1 |
| TableMinimalPerf.default | 378 | 381 | 0.99:1 |
| DividerMinimalPerf.default | 345 | 351 | 0.98:1 |
| DropdownManyItemsPerf.default | 649 | 659 | 0.98:1 |
| ListNestedPerf.default | 527 | 538 | 0.98:1 |
| ProviderMinimalPerf.default | 937 | 961 | 0.98:1 |
| TextAreaMinimalPerf.default | 472 | 489 | 0.97:1 |
| ButtonSlotsPerf.default | 505 | 527 | 0.96:1 |
| RosterPerf.default | 1095 | 1146 | 0.96:1 |
| AnimationMinimalPerf.default | 400 | 420 | 0.95:1 |
Compiled stylesI created a CodeSandbox to play with compiled styles, https://codesandbox.io/s/naughty-forest-wypey?file=/src/App.js. I see following on the arrow:
These classes are also present in build output:
Runtime stylesWorks perfectly 👍 Styles locallyHouston, we have a problem! First of all, we run Babel plugin for Storybook that is not expected (#18783), it will be fixed once #18977 will be merged:
But the issue is still there, I inserted additional logging and got: It looks that width: `${Math.SQRT2 * 6}px`,
height: `${Math.SQRT2 * 6}px`,It looks that it's the same issue as in #18903. |
| @@ -21,6 +20,10 @@ export const tooltipShorthandProps: TooltipShorthandProps[] = ['content']; | |||
|
|
|||
| const mergeProps = makeMergeProps<TooltipState>({ deepMerge: tooltipShorthandProps }); | |||
|
|
|||
| // Style values that are required for popper to properly position the tooltip | |||
| const tooltipBorderRadius = 4; // Update the root's borderRadius in useTooltipStyles.ts if this changes | |||
There was a problem hiding this comment.
can we use proper JSDoc comment or at least comment before token definition ?
There was a problem hiding this comment.
JSDoc is only used for public/exported things, right? My reasoning for the comments on the same line was to make it blindingly obvious what each constant needs to be kept in sync with.
| const popper = usePopper({ | ||
| enabled: visible, | ||
| position: state.position, | ||
| align: state.align, | ||
| offset: [0, state.offset + (state.noArrow ? 0 : arrowHeight)], | ||
| arrowPadding: theme?.global ? 2 * parseInt(tooltipBorderRadius(theme), 10) : 0, | ||
| arrowPadding: 2 * tooltipBorderRadius, |
There was a problem hiding this comment.
what does 2 do ? can we mitigate using magic numbers and assign them into self describing constant ?
There was a problem hiding this comment.
The 2 is for styling purposes, and comes from the visual spec. The name of the constant here is arrowPadding, which is defined as twice the border radius. It is unfortunate that popper requires some style-related values to be defined here instead of useTooltipStyles.
I generally agree with you about not using magic numbers, but we don't typically give names to styling constants beyond what they style (in this case, arrowPadding). I could define e.g. const tooltipBorderRadiusMultipleForArrowPadding = 2; but that seems fairly useless?
|
🎉 Handy links: |
Replace functions/constants that were shared between useTooltip/useTooltipStyles, with static values, and add comments where the values need to be kept in sync between the files.
Pull request checklist
$ yarn changeDescription of changes
A change to makeStyles broke styles that call functions to dynamically calculate style values. Tooltip had used functions/constants in a few places to align values between useTooltip and useTooltipStyles. Instead, replace those constants with static values, and add comments where the values need to be kept in sync between the files.