fix(react-avatar): Fixing focus indicator in dark theme for pie layout#23860
fix(react-avatar): Fixing focus indicator in dark theme for pie layout#23860sopranopillow merged 5 commits intomicrosoft:masterfrom
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 ccf3f15:
|
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 185ff7328615be1d8c0afd33cc8da767ea8dd891 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1287 | 1281 | 5000 | |
| Button | mount | 872 | 876 | 5000 | |
| FluentProvider | mount | 1632 | 1605 | 5000 | |
| FluentProviderWithTheme | mount | 569 | 563 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 528 | 536 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 577 | 570 | 10 | |
| MakeStyles | mount | 2024 | 1987 | 50000 | |
| SpinButton | mount | 2440 | 2435 | 5000 |
packages/react-components/react-avatar/src/components/AvatarGroup/useAvatarGroupStyles.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-avatar/src/components/AvatarGroup/useAvatarGroupStyles.ts
Outdated
Show resolved
Hide resolved
|
|
||
| // This custom focus indicator is required for the pie layout due to the clip-path applied to the root | ||
| pieFocusIndicator: createCustomFocusIndicatorStyle({ | ||
| ...shorthands.border(tokens.strokeWidthThick, 'solid', tokens.colorStrokeFocus2), |
There was a problem hiding this comment.
Changing the border thickness on focus could theoretically cause a layout shift, although it might be ok in this case (?)
Alternatively, could you use an ::after element to draw the border? Or even just use the default createFocusOutlineStyle, which uses ::after?
There was a problem hiding this comment.
sounds good, I'll try it out and see which one works best. Thanks!
There was a problem hiding this comment.
@behowell After trying it out, I will have to keep it as id. The reasoning is that I feel this way, it makes it simpler rather than adding the ::after pseudo-element, which shouldn't have any problems due to the button being completely transparent. This styling is only applied to the pie layout therefore the others work as expected. Another reason is that the border is "inside" the circle since by using clip-path the outline won't show, this is done with inline-flex because it adds box-sizing: border-box by default (also, layout shouldn't shift because of the border being applied this way). Overall, I think this is the cleanest way, but I don't mind adding the ::after and size it like the root if preferred.
There was a problem hiding this comment.
An right, I had forgotten that this is an empty button, separate from the element that contains the pie slices, right? In that case, changing its border thickness won't affect anything, and this should be fine.
There was a problem hiding this comment.
Yup, it's a plain button with transparent styling.
packages/react-components/react-avatar/src/components/AvatarGroup/useAvatarGroupStyles.ts
Outdated
Show resolved
Hide resolved
|
|
||
| // This custom focus indicator is required for the pie layout due to the clip-path applied to the root | ||
| pieFocusIndicator: createCustomFocusIndicatorStyle({ | ||
| ...shorthands.border(tokens.strokeWidthThick, 'solid', tokens.colorStrokeFocus2), |
There was a problem hiding this comment.
An right, I had forgotten that this is an empty button, separate from the element that contains the pie slices, right? In that case, changing its border thickness won't affect anything, and this should be fine.
Current Behavior
Focus indicator for the pie layout was showing up as black, this turned out to be because the actual focus indicator wasn't showing up.
New Behavior
The focus indicator for the pie layout is now using border instead of outline and is now applied to the button.