fix(makeStyles): support ordering for pseudo selectors #17669
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
Asset size changes
Baseline commit: db4ef40e7be2a30b47e73dd0f0ec9ab17b31f241 (build) |
|
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 8dd453a:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
layershifter
commented
Apr 6, 2021
| "@supports (display: table-cell){.foo{color:red;}}", | ||
| ] | ||
| `); | ||
| }); |
There was a problem hiding this comment.
Tests are not related to changes in this PR, but are still valuable.
layershifter
commented
Apr 6, 2021
| try { | ||
| if (renderer.styleElement.sheet instanceof CSSStyleSheet) { | ||
| renderer.styleElement.sheet.insertRule(ruleCSS, renderer.index); | ||
| renderer.index++; |
There was a problem hiding this comment.
We can't rely on renderer.index for .insertRule() as insertion runs for different style nodes now.
layershifter
commented
Apr 6, 2021
| ]; | ||
|
|
||
| // Renderer types | ||
|
|
||
| export type MakeStylesMatchedDefinitions = Record<string, MakeStylesResolvedRule>; | ||
| export type MakeStylesReducedDefinitions = Record<string, MakeStylesResolvedRule>; |
There was a problem hiding this comment.
This type was renamed as we don't have "matchers" anymore.
| @@ -116,11 +116,11 @@ export function ax(dir: 'ltr' | 'rtl', classNames: (string | false | undefined)[ | |||
| const resultDefinition = resultDefinitions[property]; | |||
|
|
|||
| if (isRtl) { | |||
| const rtlPrefix = isRtl && resultDefinition[2] ? RTL_PREFIX : ''; | |||
| const rtlPrefix = isRtl && resultDefinition[3] ? RTL_PREFIX : ''; | |||
There was a problem hiding this comment.
Is this the time to introduce constants?
There was a problem hiding this comment.
btw, is there a way to force-inline a constant?
There was a problem hiding this comment.
btw, is there a way to force-inline a constant?
Terser will do it for us, I created a sandbox to show it: https://codesandbox.io/s/terser-forked-txjd2?file=/src/index.js
Is this the time to introduce constants?
Great idea, I will add them in a follow up PR, in this case we will be sure that it does not affect bundle size 👍
miroslavstastny
approved these changes
Apr 6, 2021
2 tasks
|
🎉 Handy links: |
miroslavstastny
pushed a commit
to miroslavstastny/fluentui
that referenced
this pull request
May 5, 2021
* fix(makeStyles): support ordering for pseudo selectors * Change files * fix insertion * add a VR test * fix tabIndexes * update VR tests * update VR tests * update VR tests again * update VR tests * update VR tests * update VR tests
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.
$ yarn changeDescription of changes
This PR adds the concept of style buckets inspired by Compiled: style rules insertion is ordered to follow LVFHA order.
What it means on practice? The behavior of pseudo selectors becomes predictable as they will be inserted always in the same order: rules will be inserted into different style nodes are ordered.
How it works?
Styles are inserted into different nodes based on their priority, see
styleBucketOrdering. Style nodes are created on demand, a sample output is below:Implementation details
A signature of resolved rules was changed to include a style bucket name:
Why we have this in resolved rules? This allows us to perform style bucket computations during build step and avoid runtime work.
I also changed the process of insertion to DOM (
createDOMRenderer.ts), now it creates style nodes on demand to have style rules ordered.Trade-offs
Other pseudo selectors including media queries may unexpectedly clash: