Fluent-theme: clean the fluent styles so no hard coded colors are used #8098
Conversation
Cleanup of the gray110 hard coded color with palette.neutralSecondaryAlt
Vitalius1
requested review from
Jahnp,
atneik,
mikewheaton,
betrue-final-final,
phkuo,
MLoughry and
JasonGore
|
|
||
| export const BasePickerStyles = (props: IBasePickerStyleProps): Partial<IBasePickerStyles> => { | ||
| const { theme } = props; | ||
| if (!theme) { | ||
| throw new Error('Theme is undefined or null.'); |
There was a problem hiding this comment.
shouldn't this never happen? isn't a base part of fabric that we guarantee the theme?
There was a problem hiding this comment.
Some of the components don't follow this pattern in their style props to make the theme required and this happens. I believe that is something that needs to be cleaned up as well to have a consistent API across all components.
There was a problem hiding this comment.
Whether or not theming is required via props typing doesn't affect whether it's actually available. This is actually determined by the use of styled helper which will automatically inject themes prop, even if the component has made theme prop optional. I've modified this approach in Foundation by making theme prop required as part of an inherited interface.
For now, no other components are actually checking for theme's existence in styles functions, so I think it's consistent to assume it's available in styles functions.
There was a problem hiding this comment.
(The assumption here is that 'some of the components' are all components using styled. Components that use customizable or other methods cannot technically have styles functions called.)
There was a problem hiding this comment.
@JasonGore if I remove the theme check on this components I get this lint message:
Also, there are a couple of components in OUFR that are still doing the same thing. And I know that in Foundation this is fixed 👍 Thanks for the consisitency in the API that it will bring to our repo.
There was a problem hiding this comment.
Yes, this is because of the typing on the component making it optional. Other styles functions do this:
const { effects } = theme!;Which of course isn't ideal either. In either case an exception would occur. Maybe your approach is best for communicating the actual issue.
|
Size Auditor did not detect a change in bundle size for any component! |
There was a problem hiding this comment.
A mighty effort! Code changes look great @Vitalius1 . Did you do a run-through of the components during or after making all these changes? I'm happy to do that for another round of validation.
|
@Jahnp I did look through the components both during making the changes and after I've done them. There is still one more PR I want to submit with passing the font type ramp through the theme and after will need to make another full pass through the components in pair with Ben to make sure we have followed the redlines in full (minus the components that are still waiting for design). |
|
@dzearing do you mind to take a look, please? |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
Pull request checklist
$ npm run changeDescription of changes
neutralSecondaryAltfrom the deprecation status. All the colors swaps changes were approved by designers (@betrue-final-final ) and @phkuo might have to change theThemeGeneratorlogic to take into account theneutralSecondaryAltresurrection from death.boxShadowandborderRadiusvalues from FluentDepths andstyleConstantswith the ones passed throughtheme.effectsIEffectsproperties to allow more flexibility for what to pass and making it easier to use in styles. (discussed with @phkuo )Microsoft Reviewers: Open in CodeFlow