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.

Sign up for GitHub

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

Jump to bottom

[web-components] add attr for changing accent base color on design system provider #18922

Merged
merged 2 commits into from
Jul 13, 2021
Merged

[web-components] add attr for changing accent base color on design system provider #18922

merged 2 commits into from
Jul 13, 2021

Conversation

chrisdholt
Copy link
Member

Pull request checklist

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

Description of changes

Adds a new attribute enabling the easy updating of the accent-base color token from the Fluent DSP.

Focus areas to test

(optional)

@chrisdholt chrisdholt self-assigned this Jul 13, 2021
@size-auditor
Copy link

size-auditor bot commented Jul 13, 2021 •
edited
Loading

Asset size changes

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

Baseline commit: a63f33c38288c5be11443f648487db825b23ba7a (build)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 13, 2021 •
edited
Loading

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 6a27d92:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@chrisdholt chrisdholt force-pushed the users/chhol/add-attr-for-accent-base-color branch from 4a0a5df to 6a27d92 Compare July 13, 2021 18:33
@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 769 801 5000
BaseButton mount 886 887 5000
Breadcrumb mount 2580 2573 1000
ButtonNext mount 527 523 5000
Checkbox mount 1458 1479 5000
CheckboxBase mount 1269 1302 5000
ChoiceGroup mount 4601 4588 5000
ComboBox mount 953 949 1000
CommandBar mount 9931 9952 1000
ContextualMenu mount 6158 6141 1000
DefaultButton mount 1067 1072 5000
DetailsRow mount 3677 3614 5000
DetailsRowFast mount 3729 3642 5000
DetailsRowNoStyles mount 3451 3426 5000
Dialog mount 2095 2117 1000
DocumentCardTitle mount 143 144 1000
Dropdown mount 3136 3210 5000
FluentProviderNext mount 7175 7152 5000
FocusTrapZone mount 1734 1755 5000
FocusZone mount 1758 1771 5000
IconButton mount 1672 1683 5000
Label mount 333 322 5000
Layer mount 1739 1726 5000
Link mount 452 461 5000
MakeStyles mount 1800 1820 50000
MenuButton mount 1418 1431 5000
MessageBar mount 1960 1974 5000
Nav mount 3209 3170 1000
OverflowSet mount 1013 1007 5000
Panel mount 2068 2040 1000
Persona mount 817 795 1000
Pivot mount 1353 1391 1000
PrimaryButton mount 1242 1263 5000
Rating mount 7509 7421 5000
SearchBox mount 1319 1309 5000
Shimmer mount 2460 2442 5000
Slider mount 1904 1931 5000
SpinButton mount 4860 4859 5000
Spinner mount 404 407 5000
SplitButton mount 3115 3102 5000
Stack mount 481 486 5000
StackWithIntrinsicChildren mount 1463 1464 5000
StackWithTextChildren mount 4402 4392 5000
SwatchColorPicker mount 9933 9986 5000
Tabs mount 1355 1406 1000
TagPicker mount 2347 2363 5000
TeachingBubble mount 11691 11677 5000
Text mount 408 401 5000
TextField mount 1366 1336 5000
ThemeProvider mount 1162 1166 5000
ThemeProvider virtual-rerender 586 587 5000
Toggle mount 774 795 5000
buttonNative mount 113 114 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
PortalMinimalPerf.default 186 163 1.14:1
FlexMinimalPerf.default 286 266 1.08:1
RefMinimalPerf.default 245 227 1.08:1
FormMinimalPerf.default 405 379 1.07:1
ChatDuplicateMessagesPerf.default 298 280 1.06:1
LabelMinimalPerf.default 399 375 1.06:1
ReactionMinimalPerf.default 377 356 1.06:1
IconMinimalPerf.default 613 580 1.06:1
AlertMinimalPerf.default 274 260 1.05:1
AttachmentMinimalPerf.default 155 148 1.05:1
CardMinimalPerf.default 561 532 1.05:1
RadioGroupMinimalPerf.default 454 433 1.05:1
AnimationMinimalPerf.default 405 390 1.04:1
AvatarMinimalPerf.default 196 189 1.04:1
BoxMinimalPerf.default 337 326 1.03:1
ChatMinimalPerf.default 654 632 1.03:1
DividerMinimalPerf.default 352 342 1.03:1
ImageMinimalPerf.default 361 350 1.03:1
ListNestedPerf.default 548 532 1.03:1
ButtonOverridesMissPerf.default 1669 1630 1.02:1
EmbedMinimalPerf.default 4031 3967 1.02:1
ListCommonPerf.default 604 593 1.02:1
ListMinimalPerf.default 507 497 1.02:1
ProviderMergeThemesPerf.default 1647 1619 1.02:1
SplitButtonMinimalPerf.default 3731 3674 1.02:1
TreeMinimalPerf.default 784 766 1.02:1
ButtonMinimalPerf.default 167 166 1.01:1
ButtonSlotsPerf.default 527 522 1.01:1
CheckboxMinimalPerf.default 2690 2651 1.01:1
DialogMinimalPerf.default 740 731 1.01:1
HeaderMinimalPerf.default 350 347 1.01:1
LayoutMinimalPerf.default 355 352 1.01:1
SegmentMinimalPerf.default 346 342 1.01:1
TableMinimalPerf.default 398 396 1.01:1
CustomToolbarPrototype.default 3770 3737 1.01:1
ToolbarMinimalPerf.default 923 911 1.01:1
TreeWith60ListItems.default 167 166 1.01:1
DropdownMinimalPerf.default 3031 3031 1:1
GridMinimalPerf.default 333 333 1:1
InputMinimalPerf.default 1220 1223 1:1
ItemLayoutMinimalPerf.default 1201 1197 1:1
LoaderMinimalPerf.default 672 673 1:1
MenuButtonMinimalPerf.default 1604 1602 1:1
PopupMinimalPerf.default 586 585 1:1
SkeletonMinimalPerf.default 354 355 1:1
SliderMinimalPerf.default 1543 1546 1:1
TableManyItemsPerf.default 1856 1852 1:1
TooltipMinimalPerf.default 980 979 1:1
AccordionMinimalPerf.default 144 145 0.99:1
AttachmentSlotsPerf.default 1040 1050 0.99:1
DatepickerMinimalPerf.default 5228 5288 0.99:1
DropdownManyItemsPerf.default 657 667 0.99:1
HeaderSlotsPerf.default 729 737 0.99:1
MenuMinimalPerf.default 819 824 0.99:1
ProviderMinimalPerf.default 960 974 0.99:1
VideoMinimalPerf.default 614 622 0.99:1
ListWith60ListItems.default 609 623 0.98:1
StatusMinimalPerf.default 642 657 0.98:1
TextMinimalPerf.default 326 334 0.98:1
RosterPerf.default 1134 1174 0.97:1
TextAreaMinimalPerf.default 476 495 0.96:1
CarouselMinimalPerf.default 439 462 0.95:1
ChatWithPopoverPerf.default 332 353 0.94:1

EisenbergEffect
EisenbergEffect reviewed Jul 13, 2021
View reviewed changes
packages/web-components/src/design-system-provider/index.ts
*/
private accentBaseColorChanged(prev: Swatch, next: Swatch): void {
if (next !== undefined && next !== null) {
accentPalette.setValueFor(this, PaletteRGB.create(next as SwatchRGB));
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of casting makes me a bit nervous. Is it possible to get a non-RGB swatch here somehow? If not, should we type the property to SwatchRGB instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bheston thoughts on the above? This mimics the neutral change you just made :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We could update the type to SwatchRGB as well. I was trying to keep the interface the same as the design tokens, which hide the RGB portion internally. I think we may actually be able to simplify this implementation in the future, so I think in the interest of not breaking the interface, I think this is a reasonable implementation. Alternatively we could toString and reparse, which wouldn't assume the type, but I'm not sure what other types of Swatches are reasonable, so I think it's unlikely to break.

bheston
bheston approved these changes Jul 13, 2021
View reviewed changes
packages/web-components/src/design-system-provider/index.ts
*/
private accentBaseColorChanged(prev: Swatch, next: Swatch): void {
if (next !== undefined && next !== null) {
accentPalette.setValueFor(this, PaletteRGB.create(next as SwatchRGB));
Copy link
Contributor

Choose a reason for hiding this comment

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

We could update the type to SwatchRGB as well. I was trying to keep the interface the same as the design tokens, which hide the RGB portion internally. I think we may actually be able to simplify this implementation in the future, so I think in the interest of not breaking the interface, I think this is a reasonable implementation. Alternatively we could toString and reparse, which wouldn't assume the type, but I'm not sure what other types of Swatches are reasonable, so I think it's unlikely to break.

@EisenbergEffect EisenbergEffect self-requested a review July 13, 2021 21:17
@chrisdholt chrisdholt merged commit 3d08290 into microsoft:master Jul 13, 2021
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

PeterDraex pushed a commit to PeterDraex/fluentui that referenced this pull request Aug 6, 2021
@chrisdholt @PeterDraex
[web-components] add attr for changing accent base color on design sy…
43268f6 
…stem provider (microsoft#18922)

* add attribute to recreate the accentPalette via DSP

* Change files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bheston bheston bheston approved these changes

@EisenbergEffect EisenbergEffect EisenbergEffect approved these changes

@nicholasrice nicholasrice Awaiting requested review from nicholasrice

Assignees

@chrisdholt chrisdholt

Labels
web-components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chrisdholt @fabricteam @msft-fluent-ui-bot @EisenbergEffect @bheston