feat: add useCSS hook #14470
feat: add useCSS hook #14470
Conversation
layershifter
requested review from
dvdzkwsk,
dzearing,
jurokapsiar,
levithomason,
miroslavstastny,
mnajdova and
pompomon
as code owners
msft-github-bot
added
the
Fluent UI react-northstar (v0)
|
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 d0d327c:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Potential regressions comparing to master
Perf comparison
Perf tests with no regressions
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: feb91b97860c47dbe0b4eac542f6ac615dc06a47 (build) |
|
|
| import _Stylis from 'stylis'; | ||
| // @ts-ignore No typings :( | ||
| import focusVisiblePlugin from '@quid/stylis-plugin-focus-visible'; | ||
| // @ts-ignore No typings :( |
| // Types | ||
| // | ||
|
|
||
| export type UseCSSStyle = Omit<ICSSInJSStyle, 'animationName'>; |
There was a problem hiding this comment.
Do we support our animation definition in useCss? If not we do not have to omit this prop.
There was a problem hiding this comment.
No we don't support, changed to:
// Inline keyframe definitions are not supported by useCSS() hook
export type UseCSSStyle = Omit<ICSSInJSStyle, 'animationName'> & { animationName?: string };
|
|
||
| // serializeStyles() will concat all passed styles and will resolve functions | ||
| const serializedStyles = serializeStyles(resolvedStyles, stylesCache, theme); | ||
| // ".name" is not a valid CSS classname |
There was a problem hiding this comment.
Why? And why adding an "f" before it makes it valid?
There was a problem hiding this comment.
Changed to // ".name" is not a valid CSS classname as it can start from a digit, now it should be cleaner 🐱
| mount(<TestComponent styles={styles} />, getMountOptions(renderGlobal, false)); | ||
| mount(<TestComponent styles={styles} />, getMountOptions(renderGlobal, true)); | ||
|
|
||
| expect(renderGlobal).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
Wouldn't it be better if we test with window.getComputerStyle? Feels like more stable that snapshot
There was a problem hiding this comment.
It means that we will need to actually invoke Fela/Emotion to the process that can make results depend on a renderer...
| const styles = [{ color: 'red' }]; | ||
| const wrapper = mount(<TestComponent styles={styles} />); | ||
|
|
||
| expect(wrapper.find('div').prop('className')).toBeDefined(); |
There was a problem hiding this comment.
On the other hand shouldn't we also test what that className is, or what styles the component has.
There was a problem hiding this comment.
Changed it as className is stable
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
…into feat/use-css
| // Definitions | ||
| // | ||
|
|
||
| const SPECIFICITY_CLASSNAME = 'fcss'; |
There was a problem hiding this comment.
I find the previous .increase-specificity more informative than fcss. We could also consider .use-css or something as well.
Having been told to consider production size of the CSS, we could use .css which would still correlate to the useCSS call.
Not a blocker
|
Overall, this is great, it helps to abstract the css in js calls to use any centralized renderer, and extracts the important parts (rtl processing, merging, classname to style destructuring, etc). I'm wondering if there is a way to avoid all of the work on every render when it's not necessary, like deps array or string key. React memo would work for local cache but really you need more of a global cache. Without it, you'd need to build everything on every render and let the renderer use the entire object (keys/values) as the cache key which is expensive. The perf issue is pretty much the exact thing we had to fix in getClassNames (and i think Marija addressed a while ago) and still find the abuses add up (cache key invalidation cases.) If the key is a string, there's 1 key, and a bunch of edge cases would be removed. |
…eat/use-css � Conflicts: � packages/fluentui/react-bindings/package.json
| // Types | ||
| // | ||
|
|
||
| // Inline keyframe definitions are not supported by useCSS() hook |
There was a problem hiding this comment.
Should we put that note in the docs as well?
|
Perf definitely needs to be tested and proven before we can publicly ship this. For now, let's not let premature optimization hold up testing it. We need to merge and test against a couple component pages in the docs and in some production app code. Once we do this, it may fail entirely, in which case there's no need to spend time optimizing. If it passes those pragmatic checks, let's hit the perf hard. |
| const firstWrapper = mount(<TestComponent styles={[{ left: '20px' }]} />); | ||
| const firstClassName = firstWrapper.find('div').prop('className') as string; | ||
|
|
||
| mount(<TestComponent styles={[firstClassName, { color: 'red', left: '30px' }]} />, getMountOptions(renderGlobal)); |
There was a problem hiding this comment.
Would suggest testing as part of this case another component that has those classes reversed, so we can potentially find issues about this in the future if we change the logic.
packages/fluentui/docs/src/examples/components/Button/Performance/index.tsx
Outdated
Show resolved
Hide resolved
…eat/use-css � Conflicts: � packages/fluentui/docs/src/components/Sidebar/Sidebar.tsx
* feat: add useCSS hook * fix example path * fix example name * fix example names * fix lint issues * fix comment * Update packages/fluentui/react-bindings/test/hooks/useCSS-test.tsx Co-authored-by: Marija Najdova <mnajdova@gmail.com> * fix an assertion * use different selector for dev * add initial docs * improve examples, add RTL example * add a UT * fix merge issue Co-authored-by: Marija Najdova <mnajdova@gmail.com>
(cherry picked from commit f00a6cd)
An alternative for #13641.
This PR adds
useCSS()hook for Fluent UI Northstar. It allows to use CSS-in-JS to generate aclassNamethat can be applied to any component. To merge styles I used an approach inspired by Emotion'scx()function which allows merging styles and/or class names.All features like nested & preudo selectors are supported.
I've added 3 perf examples to this PR. The first is representative of usage in Teams today. The second two are for comparison to that baseline, both using
useCSS. This was done to ensureuseCSSwould not be slower than what is in react-northstar usage today.