-
Notifications
You must be signed in to change notification settings - Fork 2.9k
RFC: no functions in makeStyles() #20651
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| # RFC: No functions in `makeStyles()` | ||
|
|
||
| --- | ||
|
|
||
| @layershifter @khmakoto | ||
|
|
||
| ## Summary/problem | ||
|
|
||
| Currently `makeStyles()` allows to define style rules in two different ways: | ||
|
|
||
| ```ts | ||
| makeStyles({ | ||
| // 👇 as an object | ||
| rootA: { color: 'red' }, | ||
| // 👇 as a function | ||
| rootB: theme => ({ color: theme.tokenB }), | ||
| }); | ||
| ``` | ||
|
|
||
| - `rootB` uses a function where `theme` is typed and coupled to tokens from `@fluentui/react-theme`. This tokens shape is not extensible for customers. | ||
|
|
||
| # Proposed solution | ||
|
|
||
| - Remove functions from `makeStyles()` calls | ||
| - Export the `tokens` object with CSS variables | ||
|
|
||
| ## Export tokens separately | ||
|
|
||
| Initially we planed to support IE11 via runtime tricks, but with the deprecation of IE11, we are now able to leverage CSS Variables for tokens and theming purposes. We communicate this fact to customers so that they understand clearly what they are using. | ||
|
|
||
| The proposal is to export `tokens` as a plain object: | ||
|
|
||
| ```ts | ||
| import type { Theme } from '@fluentui/react-theme'; | ||
|
|
||
| const tokens: Theme = { | ||
| borderRadiusNone: 'var(--borderRadiusNone)', | ||
| borderRadiusSmall: 'var(--borderRadiusSmall)', | ||
| /* ... */ | ||
| }; | ||
| ``` | ||
|
|
||
| This also removes the need of using the `useTheme()` hook for customers as tokens can be accessed directly, for example: | ||
|
|
||
| ```tsx | ||
| import { tokens } from '@fluentui/react-theme'; | ||
|
|
||
| function CustomComponent() { | ||
| return <div style={{ color: tokens.borderRadiusNone /* is "var(--borderRadiusNone)" */ }} />; | ||
| } | ||
| ``` | ||
|
|
||
| That simplifies also integration with other CSS-in-JS from Fluent UI v8 or Fluent UI Northstar. | ||
|
|
||
| ## Remove functions in `makeStyles()` | ||
|
|
||
| Once `tokens` are available there is no more need for functional style rules in `makeStyles()`: | ||
|
|
||
| ```diff | ||
| import { makeStyles } from '@fluentui/react-make-styles'; | ||
| +import { tokens } from '@fluentui/react-theme'; | ||
|
|
||
| makeStyles({ | ||
| - root: theme => ({ color: theme.tokenB }), | ||
| + root: { color: tokens.tokenB }, | ||
| }); | ||
| ``` | ||
|
|
||
| ## Simplify types in `FluentProvider` | ||
|
|
||
| Currently `FluentProvider` only supports the `Theme` type from `@fluentui/react-theme`. | ||
|
|
||
| If we simplify this to `Record<string, string | number>`, we enable consumers to extend the default theme: | ||
|
|
||
| ```tsx | ||
| import { FluentProvider } from '@fluentui/react-provider'; | ||
| import { mergeThemes, teamsLightTheme, Theme } from '@fluentui/react-theme'; | ||
|
|
||
| type CustomTokens = { | ||
| tokenA: string; | ||
| }; | ||
| type CustomTheme = CustomTokens & Theme; | ||
|
|
||
| const extendedTheme: CustomTheme = mergeThemes(teamsLightTheme, { tokenA: 'red' }); | ||
|
|
||
| function App() { | ||
| return <FluentProvider theme={extendedTheme} />; | ||
| } | ||
| ``` | ||
|
|
||
| We could alternatively make the theme property in `FluentProvider` extend from `PartialTheme` if we want to ensure that the theme that is passed in always has the keys for the default tokens we provide. | ||
|
|
||
| The type of `FluentProviderProps` would then be: | ||
|
|
||
| ```ts | ||
| export interface FluentProviderProps<TTheme extends PartialTheme = PartialTheme> | ||
| extends Omit<ComponentProps<FluentProviderSlots>, 'dir'>, | ||
| Partial<FluentProviderCommons> { | ||
| theme?: TTheme; | ||
| } | ||
| ``` | ||
|
|
||
| And we could then use it as follows: | ||
|
|
||
| ```tsx | ||
| import { FluentProvider } from '@fluentui/react-provider'; | ||
| import { mergeThemes, teamsLightTheme, Theme } from '@fluentui/react-theme'; | ||
|
|
||
| type CustomTokens = { | ||
| tokenA: string; | ||
| }; | ||
| type CustomTheme = CustomTokens & Theme; | ||
|
|
||
| const extendedTheme: CustomTheme = mergeThemes(teamsLightTheme, { tokenA: 'red' }); | ||
|
|
||
| function App() { | ||
| return <FluentProvider theme={extendedTheme} />; | ||
| } | ||
| ``` | ||
|
|
||
| In both scenarios above, `FluentProvider` will still inject all customer tokens properly including scenarios with React Portals. | ||
|
|
||
| Also, in case there is an ask for it, we can decide to export a primitive component in the future (named for example `TokensProvider`) whose only purpose would be to render variables for `Record<string, string | number>`. | ||
|
|
||
| ## Using custom tokens in `makeStyles()` | ||
|
|
||
| Once tokens are injected through the `FluentProvider` they can be used in `makeStyles()`. | ||
|
|
||
| ```tsx | ||
| import { tokens } from '@fluentui/react-theme'; | ||
| import type { CustomTokens } from './custom-theme'; | ||
|
|
||
| const customTokens: CustomTokens = { | ||
| tokenA: 'var(--tokenA)', | ||
| }; | ||
|
|
||
| makeStyles({ | ||
| root: { | ||
| backgroundColor: customTokens.tokenA, | ||
| color: tokens.tokenB, | ||
| }, | ||
| }); | ||
| ``` | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
thought (out of topic): it would be nice if we could introduce more self explanatory API for consumers to create custom theme like following:
or even more explicit, with less API surface needed
WDYT?
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 like the second option and we could even make some changes to
mergeThemesto allow for this type of functionality:This would leverage our existing
mergeThemesfunctionality so we don't have to create a separatecreateCustomThemefunction.WDYT of this approach? If you like it, I can go ahead and update the RFC with this info.
Uh oh!
There was an error while loading. Please reload this page.
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'm spinning this up into a separate issue so we can continue the conversation without blocking this PR.
#21181