fix(makeStyles): handle RTL properly #17549
Merged
Conversation
This file contains 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
|
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 88d58b9:
|
Asset size changes
Baseline commit: c2331d66c74be1b3e0ed413b89d4f795ee5581a1 (build) |
Perf Analysis
All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
ling1726
reviewed
Mar 25, 2021
Comment on lines
-28
to
-29
| // arguments are parsed manually to avoid double loops as TS & Babel transforms rest via an additional loop | ||
| // @see https://babeljs.io/docs/en/babel-plugin-transform-parameters |
There was a problem hiding this comment.
Is this perf opimization no longer true ?
There was a problem hiding this comment.
Yes, as signature of ax() in make-styles changed (now it accepts an array as a second param).
layershifter
commented
Mar 25, 2021
| @@ -1,3 +1,2 @@ | |||
| export { createCSSVariablesProxy } from './createCSSVariablesProxy'; | |||
| export { resolveDefinitions } from './resolveDefinitions'; | |||
There was a problem hiding this comment.
Cruft, from previous implementation of makeStyles()
layershifter
commented
Mar 25, 2021
| @@ -394,7 +390,7 @@ describe('resolveStyleRules', () => { | |||
| animationDuration: '5s', | |||
| }), | |||
| ).toMatchInlineSnapshot(` | |||
| @-webkit-keyframes f13owpa8 { | |||
| @-webkit-keyframes f1q8eu9e { | |||
There was a problem hiding this comment.
Hashes of keyframes have been changed because of a change in cssifyObject() that always adds trailing commas now.
layershifter
requested review from
behowell,
khmakoto and
kubkon
as code owners
| import { cssifyObject } from './utils/cssifyObject'; | ||
|
|
||
| export function compileStaticCSS(property: string, value: MakeStyles): string { | ||
| const cssRule = `${property} {${cssifyObject(value)}}`; | ||
| return compileCSSRule(cssRule); | ||
| return compileCSSRules(cssRule)[0]; |
There was a problem hiding this comment.
What is the story for RTL in static CSS?
There was a problem hiding this comment.
Automatic transformers are not supported as we don't know how global classes will be applied later. For example:
body { left: 10px }|
|
||
| // eslint-disable-next-line guard-for-in | ||
| for (const slotName in resolvedStyles) { | ||
| const slotClasses = renderer.insertDefinitions(dir, resolvedStyles[slotName]); |
There was a problem hiding this comment.
It is renderer's responsibility to compute the classnames, but the rest of the code expect the classnames are stable and atomic. Where do we have this contract described and tested?
There was a problem hiding this comment.
It is not described. For me it's unclear path yet...
- Will we implement CSS extraction to .css files?
- How will look build time transform?
- Where is the balance between memory and perf for runtime/build time?
I don't have answers and thus don't want to make a strict contract there yet...
packages/react-examples/src/react-make-styles/RTL/RTL.stories.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
miroslavstastny
approved these changes
Mar 30, 2021
22 tasks
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
This was referenced Apr 6, 2021
miroslavstastny
added a commit
to miroslavstastny/fluentui
that referenced
this pull request
May 5, 2021
* fix(makeStyles): handle RTL properly * Change files * fix test, add export to avoid explosion in all components * fix lint * fix tests, fix build issues * fix typings, ignore eslint * fixes to compileCSS, unit tests * fixes to cssifyObject(), update useBadgeStyles & useCardStyles * rework impl * Change files * remove todo * complete todos * add a UT * add VR tests * use smaller screenshots * Update packages/make-styles/src/ax.ts Co-authored-by: Miroslav Stastny <mistastn@microsoft.com> * Update packages/react-examples/src/react-make-styles/RTL/RTL.stories.tsx Co-authored-by: Miroslav Stastny <mistastn@microsoft.com> * add a warning and UT for unregistered sequence * fix insertion for RTL * fix lint error Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pull request checklist
Addresses an existing issue: Fixes$ yarn changeTL;DR
This PR fixes RTL handling in
makeStyles()and makesax()context aware, so it will also handle it properly.RTL classnames
We can only use different classes for LTR & RTL if we want to avoid collisions, see CodeSandbox for example. This PR follows this approach, we will emit following CSS:
(results in 👇)
Previously,
makeStyles()always returned LTR classes, this PR fixes it as classes for LTR and RTL are resolved separately (resolvedClasses&resolvedClassesRtl).ax() changes
The signature of
ax()function@fluentui/make-styleshave been changed as spreads are complied into additional loops:Sample TS compiler output
In
@fluentui/react-make-styles, but nowax()consumes a React context to be aware of text direction. I will rename it intouseAx()in a separate PR, in this PR bothax()anduseAx()are exported.:globalchangesBehavior for
:global()pseudo selector have been changed.Before
After
Now output will be the same.
We should use
makeStaticStyles()to emit global styles:makeStyles()emits theme specific styles that can be different for different themes. An example of possible issue before the change:Testing
PR contains a lot of new unit tests, new stories for
@fluentui/react-make-stylesand VR tests.