RFC: makeStyles() API changes #17076
Conversation
layershifter
changed the title
|
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 3744775:
|
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Co-authored-by: ling1726 <lingfan.gao@microsoft.com>
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: a93b07b3154b68801090b70c4911f49ab25ecb12 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
rfcs/convergence/make-overrides.md
Outdated
| rootShape20: { width: '20px', height: '20px' }, | ||
| rootShape24: { width: '24px', height: '24px' }, | ||
| rootShape28: { width: '28px', height: '28px' }, | ||
| }) |
There was a problem hiding this comment.
[non-blocking] I do quite like this pattern of defining styles ahead of time, then conditionally applying them below. We should measure, but my intuition says that this will have good opportunity for optimization and gets us closer to static styles.
I also find this more intuitive than the matcher approach 👍 I think this will be easier for devs to understand and other teams to use in their products.
There was a problem hiding this comment.
I also find this more intuitive than the matcher approach 👍 I think this will be easier for devs to understand and other teams to use in their products.
There is one interesting difference:
- with matchers (i.e.
makeStyles()) you can just execute a function and get classes back - with
makeOverrides()you need to execute component's code to understand applied styles
I am considering this an implementation difference that should not block us from moving forward.
There was a problem hiding this comment.
Question: What prevents us from abstracting the component code into a separate function so that we can, like you said, just execute a function and get classes back?
There was a problem hiding this comment.
Question: What prevents us from abstracting the component code into a separate function so that we can, like you said, just execute a function and get classes back?
function getRootClasses(state, classes) {
return ax(classes.root, state.primary && classes.primary, /* etc. */)
}Do you mean something like this?
|
I think it is awesome that we are hyper-focused on performance. My team cannot adopt the new theming system (and as a result, Fluent UI components) unless the performance is very close to CSS modules. From this perspective, I like this updated spec. In the spec, can you add an example that shows how one can specify selectors like |
|
In this new styling/theming system, what is the recommended way for customers to customize the styling of Fluent components? |
| ```tsx | ||
| import { ax, makeOverrides } from "@fluentui/react-make-styles"; | ||
|
|
||
| const useStyles = makeStyles({ |
There was a problem hiding this comment.
I can't find it in the spec anymore, but when I last looked at it, the theming object was passed to each individual 'slot'. Can it just be an argument to a callback for makeOverrides first argument?
I.e.
const useStyles = makeOverrides((theme: ITheme) => {
root: ...,
other: ...
}));
instead of
const useStyles = makeOverrides({
root: (theme: ITheme) => { ... },
other: (theme: ITheme) => { ... }
});
Saves some lambda creation at runtime and in my opinion is a bit more intuitive.
There was a problem hiding this comment.
Can it just be an argument to a callback for makeOverrides first argument?
We are considering this option, but there are few open questions.
1. IE11 should have decent performance if there will be a decision to include it into supported browsers, in this case build step can split styles to runtime dependent:
// ⚠️ not real code, just shows an idea
const useStyles = makeStyles([
[null, theme => ({ color: "red", background: theme.colors.red })]
]);
// =>
const useStyles = makeStyles([
[null, { color: "red" }],
[null, theme => ({ background: theme.colors.red })] // only "theme" usages are inside this closure
]);2. Our work related to tokens is not finished yet, we may decide to go with CSS variables (and some magic shim for IE11, if required). But, the open question is why we should have theme and closures at all?
const useStyles = makeStyles([
// current option, simpler implementation for IE11
[null, theme => ({ background: theme.colors.red })],
// "vars" can be an object will all CSS variables that are in theme to enforce type safety
[null, { background: vars.colors.red }],
// can we just use CSS variables?
[null, { background: "--theme-colors-red" }]
]);Saves some lambda creation at runtime and in my opinion is a bit more intuitive.
Once we will have build time transforms, there will be no closures for modern browsers at all in runtime 🐱 Currently this work is done on an initial render, for example:
const useLabelStyles = makeStyles<AvatarState>([
[
null,
tokens => ({
color: tokens.alias.color.neutral.neutralForeground3,
background: tokens.alias.color.neutral.neutralBackground6
})
]
]);Will produce this CSS:
To clarify: proposed API changes will not affect syntax support in any way. The goal is to eliminate "matchers level" that does not work so well as it was expected 😭 An original makeStyles.md spec includes an example of selectors usage in "Basic algorithm", but it's not clear agree 👍 We will have more detailed docs in Storybook and Currently following selectors are supported (we will include these as an examples to docs 🐱 ): const useStyles = makeStyles([
[
null,
{
":hover": { color: "red" }, // and other pseudo states
".classname": { color: "red" },
"& .classname": { color: "red" },
"& .classname > div": { color: "red" },
":global(body)": { color: "red" } // :global is also supported
}
]
]); |
|
rfcs/convergence/make-overrides.md
Outdated
| // 👎 It might be tricky find proper names to express definition names | ||
| // (we can end with "rootPrimaryCirclularGhostEtc.") |
There was a problem hiding this comment.
Not blocking by any means for this PR but we might want to find a convention for how we name things.
For example, we could go with [slot] [alphabetically ordered states].
So, for the example above that would be rootCircularGhostPrimary.
Just food for thought 😄.
rfcs/convergence/make-overrides.md
Outdated
| rootShape20: { width: '20px', height: '20px' }, | ||
| rootShape24: { width: '24px', height: '24px' }, | ||
| rootShape28: { width: '28px', height: '28px' }, | ||
| }) |
There was a problem hiding this comment.
Question: What prevents us from abstracting the component code into a separate function so that we can, like you said, just execute a function and get classes back?
Co-authored-by: Makoto Morimoto <humbertomakotomorimoto@gmail.com>
Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
…ayershifter-patch-1
* RFC: makeStyles() replacement * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md * Update rfcs/convergence/make-overrides.md * Update make-overrides.md * Create make-overrides.md * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: ling1726 <lingfan.gao@microsoft.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: Makoto Morimoto <humbertomakotomorimoto@gmail.com> * Update rfcs/convergence/make-overrides.md Co-authored-by: Miroslav Stastny <mistastn@microsoft.com> * update formatting Co-authored-by: ling1726 <lingfan.gao@microsoft.com> Co-authored-by: Makoto Morimoto <humbertomakotomorimoto@gmail.com> Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
This RFC is about proposed API changes to
makeStyles(). RFC includes a description of potential performance issues and proposed design changes that should improve it.HTML rendered version can be viewed here
A PR with implementation