-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
(web-components) Web component visual update #19263
(web-components) Web component visual update #19263
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 b482de5:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: bdbc8b19e6c576ff29545b4ea132f6f9f57378e1 (build) |
📊 Bundle size report🤖 This report was generated against bdbc8b19e6c576ff29545b4ea132f6f9f57378e1 |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 1095 | 1046 | 5000 | |
BaseButton | mount | 1057 | 1089 | 5000 | |
Breadcrumb | mount | 3055 | 3171 | 1000 | |
ButtonNext | mount | 561 | 581 | 5000 | |
Checkbox | mount | 1838 | 1745 | 5000 | |
CheckboxBase | mount | 1529 | 1511 | 5000 | |
ChoiceGroup | mount | 5453 | 5574 | 5000 | |
ComboBox | mount | 1134 | 1114 | 1000 | |
CommandBar | mount | 11781 | 12054 | 1000 | |
ContextualMenu | mount | 7468 | 7401 | 1000 | |
DefaultButton | mount | 1293 | 1335 | 5000 | |
DetailsRow | mount | 4299 | 4294 | 5000 | |
DetailsRowFast | mount | 4330 | 4243 | 5000 | |
DetailsRowNoStyles | mount | 4044 | 4021 | 5000 | |
Dialog | mount | 2702 | 2755 | 1000 | |
DocumentCardTitle | mount | 192 | 205 | 1000 | |
Dropdown | mount | 3695 | 3743 | 5000 | |
FluentProviderNext | mount | 3653 | 3679 | 5000 | |
FluentProviderWithTheme | mount | 218 | 226 | 10 | |
FluentProviderWithTheme | virtual-rerender | 116 | 120 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 278 | 272 | 10 | |
FocusTrapZone | mount | 2045 | 2001 | 5000 | |
FocusZone | mount | 2030 | 1991 | 5000 | |
IconButton | mount | 2069 | 2059 | 5000 | |
Label | mount | 392 | 405 | 5000 | |
Layer | mount | 3443 | 3250 | 5000 | |
Link | mount | 570 | 556 | 5000 | |
MakeStyles | mount | 2002 | 1994 | 50000 | |
MenuButton | mount | 1677 | 1727 | 5000 | |
MessageBar | mount | 2130 | 2165 | 5000 | |
Nav | mount | 3670 | 3568 | 1000 | |
OverflowSet | mount | 1206 | 1219 | 5000 | |
Panel | mount | 2690 | 2520 | 1000 | |
Persona | mount | 957 | 996 | 1000 | |
Pivot | mount | 1687 | 1692 | 1000 | |
PrimaryButton | mount | 1480 | 1567 | 5000 | |
Rating | mount | 9136 | 9364 | 5000 | |
SearchBox | mount | 1542 | 1591 | 5000 | |
Shimmer | mount | 3049 | 3040 | 5000 | |
Slider | mount | 2225 | 2311 | 5000 | |
SpinButton | mount | 5714 | 5728 | 5000 | |
Spinner | mount | 490 | 489 | 5000 | |
SplitButton | mount | 3657 | 3636 | 5000 | |
Stack | mount | 595 | 616 | 5000 | |
StackWithIntrinsicChildren | mount | 1946 | 2021 | 5000 | |
StackWithTextChildren | mount | 5483 | 5495 | 5000 | |
SwatchColorPicker | mount | 12265 | 11951 | 5000 | |
Tabs | mount | 1637 | 1639 | 1000 | |
TagPicker | mount | 3014 | 3046 | 5000 | |
TeachingBubble | mount | 14744 | 15054 | 5000 | |
Text | mount | 528 | 497 | 5000 | |
TextField | mount | 1659 | 1612 | 5000 | |
ThemeProvider | mount | 1373 | 1345 | 5000 | |
ThemeProvider | virtual-rerender | 723 | 684 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 2140 | 2160 | 5000 | |
Toggle | mount | 986 | 1004 | 5000 | |
buttonNative | mount | 158 | 138 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
RefMinimalPerf.default | 307 | 258 | 1.19:1 |
LabelMinimalPerf.default | 480 | 423 | 1.13:1 |
BoxMinimalPerf.default | 463 | 427 | 1.08:1 |
LoaderMinimalPerf.default | 851 | 787 | 1.08:1 |
FlexMinimalPerf.default | 339 | 318 | 1.07:1 |
HeaderSlotsPerf.default | 921 | 861 | 1.07:1 |
ListMinimalPerf.default | 631 | 591 | 1.07:1 |
TooltipMinimalPerf.default | 1209 | 1135 | 1.07:1 |
ChatDuplicateMessagesPerf.default | 381 | 359 | 1.06:1 |
GridMinimalPerf.default | 411 | 386 | 1.06:1 |
MenuButtonMinimalPerf.default | 1936 | 1834 | 1.06:1 |
ReactionMinimalPerf.default | 467 | 440 | 1.06:1 |
AttachmentMinimalPerf.default | 199 | 190 | 1.05:1 |
DividerMinimalPerf.default | 438 | 419 | 1.05:1 |
ImageMinimalPerf.default | 465 | 444 | 1.05:1 |
TextMinimalPerf.default | 424 | 404 | 1.05:1 |
ToolbarMinimalPerf.default | 1144 | 1086 | 1.05:1 |
AlertMinimalPerf.default | 335 | 322 | 1.04:1 |
InputMinimalPerf.default | 1505 | 1452 | 1.04:1 |
ListCommonPerf.default | 777 | 748 | 1.04:1 |
ProviderMinimalPerf.default | 1384 | 1332 | 1.04:1 |
RadioGroupMinimalPerf.default | 536 | 513 | 1.04:1 |
SplitButtonMinimalPerf.default | 4940 | 4744 | 1.04:1 |
CardMinimalPerf.default | 664 | 642 | 1.03:1 |
FormMinimalPerf.default | 509 | 492 | 1.03:1 |
HeaderMinimalPerf.default | 438 | 425 | 1.03:1 |
ListWith60ListItems.default | 813 | 790 | 1.03:1 |
MenuMinimalPerf.default | 982 | 950 | 1.03:1 |
PortalMinimalPerf.default | 198 | 192 | 1.03:1 |
VideoMinimalPerf.default | 766 | 743 | 1.03:1 |
AvatarMinimalPerf.default | 238 | 234 | 1.02:1 |
EmbedMinimalPerf.default | 4889 | 4793 | 1.02:1 |
ItemLayoutMinimalPerf.default | 1445 | 1416 | 1.02:1 |
ProviderMergeThemesPerf.default | 1939 | 1909 | 1.02:1 |
IconMinimalPerf.default | 726 | 714 | 1.02:1 |
CustomToolbarPrototype.default | 4856 | 4779 | 1.02:1 |
AttachmentSlotsPerf.default | 1248 | 1233 | 1.01:1 |
LayoutMinimalPerf.default | 433 | 429 | 1.01:1 |
ListNestedPerf.default | 638 | 632 | 1.01:1 |
RosterPerf.default | 1367 | 1360 | 1.01:1 |
PopupMinimalPerf.default | 688 | 682 | 1.01:1 |
SegmentMinimalPerf.default | 410 | 404 | 1.01:1 |
DialogMinimalPerf.default | 847 | 851 | 1:1 |
DropdownMinimalPerf.default | 3850 | 3843 | 1:1 |
SkeletonMinimalPerf.default | 421 | 421 | 1:1 |
TableMinimalPerf.default | 482 | 480 | 1:1 |
TextAreaMinimalPerf.default | 596 | 596 | 1:1 |
ChatMinimalPerf.default | 742 | 753 | 0.99:1 |
ChatWithPopoverPerf.default | 475 | 481 | 0.99:1 |
DatepickerMinimalPerf.default | 7102 | 7141 | 0.99:1 |
SliderMinimalPerf.default | 1964 | 1992 | 0.99:1 |
ButtonMinimalPerf.default | 186 | 190 | 0.98:1 |
ButtonOverridesMissPerf.default | 2077 | 2110 | 0.98:1 |
ButtonSlotsPerf.default | 635 | 648 | 0.98:1 |
StatusMinimalPerf.default | 787 | 805 | 0.98:1 |
TreeMinimalPerf.default | 928 | 950 | 0.98:1 |
TreeWith60ListItems.default | 204 | 209 | 0.98:1 |
TableManyItemsPerf.default | 2178 | 2248 | 0.97:1 |
CarouselMinimalPerf.default | 507 | 526 | 0.96:1 |
AccordionMinimalPerf.default | 170 | 179 | 0.95:1 |
CheckboxMinimalPerf.default | 3047 | 3194 | 0.95:1 |
DropdownManyItemsPerf.default | 790 | 828 | 0.95:1 |
AnimationMinimalPerf.default | 457 | 492 | 0.93: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.
yarn.lock changes approved
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.
Looks good. A few nit picks, but nothing breaking. I cloned your repo and smoke tested.
packages/web-components/src/accordion/accordion-item/accordion-item.styles.ts
Show resolved
Hide resolved
:host([circular]) .control { | ||
border-radius: 100px; | ||
padding: 0 calc(${designUnit} * 1px); | ||
height: calc((${heightNumber} - (${designUnit} * 3)) * 1px); | ||
min-width: calc((${heightNumber} - (${designUnit} * 3)) * 1px); | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
box-sizing: border-box; | ||
} |
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.
This feels a little arbitrary for width and height. I get how you're trying to match up to the height of the current badge, but the logic to get there isn't tied to the existing height. If either heightNumber or designUnit change, or if --type-ramp-minus-1-line-height changes, it will no longer be the same height as the current badge. This could be simplified by just setting width using the line-height. There is an extra designUnit of padding for width than height, so you need to adjust that but overall, it would simplify the styles.
:host([circular]) .control { border-radius: 100px; min-width: calc(var(--type-ramp-minus-1-line-height) - calc(var(--design-unit) * 1px)); display: flex; align-items: center; justify-content: center; }
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.
It's not preserving line breaks, but this method would remove the need to edit padding, add a height or add box-sizing. It will maintain the same height of the current badge if heightNumber, designUnit, or --type-ramp-minus-1-line-height changes.
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.
We'll clean this up a bit more when we fully implement density, but I reworked it using most of your suggestions.
|
||
:host(.checked:enabled:hover) .control { | ||
background: ${accentFillHover}; | ||
border-color: transparent; |
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.
The border-color here and the one below aren't needed since they'll inherit from :host(.checked) .control
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.
The :host(:enabled:hover)
selector is more specific than :host(.checked)
, so without this the border is inheriting the grey outline from the unchecked style.
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.
Question (I haven’t pulled down) - this is an ARIA checkbox and not an input. Does an ‘enabled’ selector work? I’d have the same question for radio if used considering the implementation.
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.
This is working. It looks like it probably falls under the "form-associated custom element" portion. Good to keep an eye out for though. I wish this were common across all elements.
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.
Does this work in FF and safari? Those browsers don't support custom element form-association
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.
Turns out it did not. I've updated these controls and checked that others were not using selectors like this that wouldn't work.
@@ -5,5 +5,6 @@ | |||
margin: 8px; | |||
height: 100%; | |||
background: var(--fill-color); | |||
color: var(--neutral-foreground-rest); |
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.
@bheston I think we'll want to pull this as it hides places we should probably have a good default.
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.
The reason I added this is that the two go hand in hand. This is what the design-system-provider does, and the Storybook wrapper is essentially acting as the DSP by specifying the background color at all. I think this is safe because it's intended for plain text on the page, like the headings of the component samples. It should never apply into a component, but if we want a place to test something like that I think Storybook is not that place. If it is, we should also remove the background color here.
This question has come up a few times, and I think having a default foreground color is actually a good idea. Keeping it paired so that if a section were updated the foreground would change. That's how the design tokens would work now as well. It's the default way we can handle contrast requirements. Otherwise for dark mode testing this text is black on grey.
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.
I can understand this, but while it's not intended to apply to components, it often does, most often with instances of slotted content in the light DOM. This leads to places where we assume that something is styled because the native elements inherit - but I don't think we should assume that all app and website authors will always do this, or that it's desired.
If the goal is to ensure that the common elements which make up the page outside the components theme accordingly, we can address that by swapping this for areas where we target most of those selectors - primarily headings. That can create issues if we're slotting heading content, but it's far less egregious than setting color on the body and having nearly every element inherit that color. That makes it very difficult to find edge cases where a good default could have been set or served.
If there were a way to ensure that non-light-DOM instances of text did not inherit this, then I would agree, but that's currently not the case. Given the way that CSS works and our goals for the storybook - a development site for testing the components and Adaptive UI, I think that headings theming are secondary and the implementation should play more towards our greatest needs for storybook.
Given that, would it seem reasonable to update this to apply the color to heading elements rather than the body to avoid the inheritance issue while ensuring that the headings for stories show up as expected?
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.
I think the latest update does a reasonable job of catching the elements we expect without reaching into anything we don't.
@khamudom this is actually by design: https://w3c.github.io/aria-practices/#kbd_disabled_controls |
I think the lines you are seeing are dividers slotted within the menu to create clear breaks between the functionality deltas, which may be leading to the confusion. @bheston can correct if that's not the case - if it is, I think it's as expected. One question I do have on this control though is if the |
@khamudom do you mean like a box or circle around the check or dot like a regular checkbox or radio button? If so, this is by design that it's the glyph only without the "control" border. |
I do think it's valuable to have the differentiation they've added between check and radio here. I agree that my first thought of the radio was that it was very small, but I suggest we go with it for now. This is one of those areas I think the platform should evolve as a whole. Edge has taken this change as-is in the native menus and xaml and Windows will have it. If we get feedback that it's not strong enough we can easily revise. At least it's not a commonly used option. |
Yes the box and border around the indicators |
Cool - if this has been brought in across other Fluent surfaces I think that makes sense. I wasn't sure if we were adopting a design or pushing forward. Good to go! |
@khamudom yeah, this would be by design as it's the updated design. I'll add that I think it's far preferred to having the menu items appear as though they have actionable controls within them. That could potentially be confusing because it may infer that users should be able to navigate to those items, when thats not actually accurate. The entire item represents either checkbox or radio behavior - this visual better serves the control, its behavior, and its intent. |
Adjusted icons to correct size Removed states from card Fixed input placeholder contrast Removed deprecated design tokens
Design token purpose cleanup Some Storybook example cleanup
Fix foregroundOnAccent active state color
Storybook cleanup
Added floating color for select and combobox Fixed tooltip style
ec16c9c
to
6db3fc1
Compare
Added a docs page descriping styling using design tokens. Fixed some Card samples to better illustrate using the new design tokens.
|
||
The most common need for setting a token is to switch between light and dark mode. | ||
|
||
- `baseLayerLuminance`: Set to `StandardLuminance.DarkMode` to switch into dark mode. That's it. You're welcome. |
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.
I get the intent here, but it's probably best we leave this off
- `baseLayerLuminance`: Set to `StandardLuminance.DarkMode` to switch into dark mode. That's it. You're welcome. | |
- `baseLayerLuminance`: Set to `StandardLuminance.DarkMode` to switch into dark mode. |
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.
LET'S GO!
Huge PR and a lot of effort. Thank you @bheston!
* Initial update of Fluent visual styling * Added and updated color recipes Added `circular` badge styling and example Removed `appearance` selectors Converted elevation to design tokens * Cleaned up disabled styles Cleaned up focus states Fixed text input width Updated tree view components * Update a few icons * Updated anchor and button 'disabled' styling Updated skeleton to use recipes Updated tooltip to correct colors Fixed number field spinner color * Change files * Review comments and clean up * Added 'stealth' Select appearance * Adjust accordion padding, storybook color, and light mode level * Cleaned up color recipes and application Adjusted icons to correct size Removed states from card Fixed input placeholder contrast Removed deprecated design tokens * Color recipe cleanup Design token purpose cleanup Some Storybook example cleanup * Cleanup log * Minor definitions and import fixes * Fix one more import issue * Apply updates made in FAST * Fix some button style details * Fix enabled selector rules for Firefox Fix foregroundOnAccent active state color * Rebase and cleanup for token deprecation and sorting * Updated gradient design tokens for consistent API * Fix some token exports and gradient recipe issues * Fix direction handling and nested menus * Clean up design token interface * Flipper style update to look not disabled Storybook cleanup * Fixed disabled button style Added floating color for select and combobox Fixed tooltip style * Added design tokens for neutral and accent bae colors. Added a docs page descriping styling using design tokens. Fixed some Card samples to better illustrate using the new design tokens. * Change files * Update card.html prettier for card * Run prettier * Docs update Co-authored-by: Chris Holt <chhol@microsoft.com>
Pull request checklist
$ yarn change
Description of changes
Updated the web component visual style to coordinate with Windows 11.
Focus areas to test
Visual update, including mouse and keyboard interactive states.