-
Notifications
You must be signed in to change notification settings - Fork 65
fix: handle nesting in comma separated selectors #207
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "patch", | ||
| "comment": "fix: handle nesting in comma separated selectors", | ||
| "packageName": "@griffel/core", | ||
| "email": "[email protected]", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "patch", | ||
| "comment": "chore: adopt API changes from @griffel/core", | ||
| "packageName": "@griffel/webpack-extraction-plugin", | ||
| "email": "[email protected]", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ import { normalizeNestedProperty } from './utils/normalizeNestedProperty'; | |
| export interface CompileCSSOptions { | ||
| className: string; | ||
|
|
||
| pseudo: string; | ||
| selectors: string[]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would the length of selectors ever exceed 2 ? It seems that the only case we are handling here is pseudo selector + element. It shouldn't be possible to go further since it would be invalid css ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please provide an example? |
||
| media: string; | ||
| layer: string; | ||
| support: string; | ||
|
|
@@ -60,50 +60,58 @@ export function compileCSSRules(cssRules: string): string[] { | |
| return rules; | ||
| } | ||
|
|
||
| export function compileCSS(options: CompileCSSOptions): [string /* ltr definition */, string? /* rtl definition */] { | ||
| const { className, media, layer, pseudo, support, property, rtlClassName, rtlProperty, rtlValue, value } = options; | ||
| function createCSSRule(classNameSelector: string, cssDeclaration: string, pseudos: string[]): string { | ||
| let globalSelector = ''; | ||
| let cssRule = cssDeclaration; | ||
|
|
||
| const classNameSelector = `.${className}`; | ||
| const cssDeclaration = Array.isArray(value) | ||
| ? `{ ${value.map(v => `${hyphenateProperty(property)}: ${v}`).join(';')}; }` | ||
| : `{ ${hyphenateProperty(property)}: ${value}; }`; | ||
| if (pseudos.length > 0) { | ||
| cssRule = pseudos.reduceRight((acc, selector, index) => { | ||
| // Should be handled by namespace plugin of Stylis, is buggy now | ||
| // Issues are reported: | ||
| // https://github.com/thysultan/stylis.js/issues/253 | ||
| // https://github.com/thysultan/stylis.js/issues/252 | ||
| if (selector.indexOf(':global(') === 0) { | ||
| // 👇 :global(GROUP_1)GROUP_2 | ||
| const GLOBAL_PSEUDO_REGEX = /global\((.+)\)(.+)?/; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe hoist the regex so it doesn't get created on every iteration ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, but in a followup I will get it of this part anyway (I will replace this part with a stylis plugin). |
||
| const result = GLOBAL_PSEUDO_REGEX.exec(selector)!; | ||
|
|
||
| let rtlClassNameSelector: string | null = null; | ||
| let rtlCSSDeclaration: string | null = null; | ||
| globalSelector = result[1] + ' '; | ||
| const restPseudo = result[2] || ''; | ||
|
|
||
| if (rtlProperty && rtlClassName) { | ||
| rtlClassNameSelector = `.${rtlClassName}`; | ||
| rtlCSSDeclaration = Array.isArray(rtlValue) | ||
| ? `{ ${rtlValue.map(v => `${hyphenateProperty(rtlProperty)}: ${v}`).join(';')}; }` | ||
| : `{ ${hyphenateProperty(rtlProperty)}: ${rtlValue}; }`; | ||
| } | ||
| // should be normalized to handle ":global(SELECTOR) &" | ||
| const normalizedPseudoSelector = normalizePseudoSelector(restPseudo); | ||
|
|
||
| let cssRule = ''; | ||
| if (normalizedPseudoSelector === '') { | ||
| return acc; | ||
| } | ||
|
|
||
| // Should be handled by namespace plugin of Stylis, is buggy now | ||
| // Issues are reported: | ||
| // https://github.com/thysultan/stylis.js/issues/253 | ||
| // https://github.com/thysultan/stylis.js/issues/252 | ||
| if (pseudo.indexOf(':global(') === 0) { | ||
| // 👇 :global(GROUP_1)GROUP_2 | ||
| const GLOBAL_PSEUDO_REGEX = /global\((.+)\)(.+)?/; | ||
| const [, globalSelector, restPseudo = ''] = GLOBAL_PSEUDO_REGEX.exec(pseudo)!; | ||
| return `${normalizedPseudoSelector} { ${acc} }`; | ||
| } | ||
|
Comment on lines
+70
to
+89
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved the hack with small changes, will replace it with a Stylis plugin in a follow up PR. |
||
|
|
||
| // should be normalized to handle ":global(SELECTOR) &" | ||
| const normalizedPseudo = normalizeNestedProperty(restPseudo.trim()); | ||
| return `${normalizePseudoSelector(selector)} { ${acc} }`; | ||
| }, cssDeclaration); | ||
| } | ||
|
|
||
| const ltrRule = `${classNameSelector}${normalizedPseudo} ${cssDeclaration}`; | ||
| const rtlRule = rtlProperty ? `${rtlClassNameSelector}${normalizedPseudo} ${rtlCSSDeclaration}` : ''; | ||
| return `${globalSelector}${classNameSelector}{${cssRule}}`; | ||
| } | ||
|
|
||
| cssRule = `${globalSelector} { ${ltrRule}; ${rtlRule} }`; | ||
| } else { | ||
| const normalizedPseudo = normalizePseudoSelector(pseudo); | ||
| export function compileCSS(options: CompileCSSOptions): [string /* ltr definition */, string? /* rtl definition */] { | ||
| const { className, media, layer, selectors, support, property, rtlClassName, rtlProperty, rtlValue, value } = options; | ||
|
|
||
| const classNameSelector = `.${className}`; | ||
| const cssDeclaration = Array.isArray(value) | ||
| ? `${value.map(v => `${hyphenateProperty(property)}: ${v}`).join(';')};` | ||
| : `${hyphenateProperty(property)}: ${value};`; | ||
|
|
||
| let cssRule = createCSSRule(classNameSelector, cssDeclaration, selectors); | ||
|
|
||
| cssRule = `${classNameSelector}{${normalizedPseudo} ${cssDeclaration}};`; | ||
| if (rtlProperty && rtlClassName) { | ||
| const rtlClassNameSelector = `.${rtlClassName}`; | ||
| const rtlCSSDeclaration = Array.isArray(rtlValue) | ||
| ? `${rtlValue.map(v => `${hyphenateProperty(rtlProperty)}: ${v}`).join(';')};` | ||
| : `${hyphenateProperty(rtlProperty)}: ${rtlValue};`; | ||
|
|
||
| if (rtlProperty) { | ||
| cssRule = `${cssRule}; ${rtlClassNameSelector}{${normalizedPseudo} ${rtlCSSDeclaration}};`; | ||
| } | ||
| cssRule += createCSSRule(rtlClassNameSelector, rtlCSSDeclaration, selectors); | ||
| } | ||
|
|
||
| if (media) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,19 +2,22 @@ import { getStyleBucketName } from './getStyleBucketName'; | |
|
|
||
| describe('getStyleBucketName', () => { | ||
| it('returns bucketName based on mapping', () => { | ||
| expect(getStyleBucketName(' :link', '', '', '')).toBe('l'); | ||
| expect(getStyleBucketName(' :hover ', '', '', '')).toBe('h'); | ||
| expect(getStyleBucketName([], '', '', '')).toBe('d'); | ||
| expect(getStyleBucketName(['.foo'], '', '', '')).toBe('d'); | ||
|
Comment on lines
+5
to
+6
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New test cases. |
||
|
|
||
| expect(getStyleBucketName(':link', '', '', '')).toBe('l'); | ||
| expect(getStyleBucketName(':visited', '', '', '')).toBe('v'); | ||
| expect(getStyleBucketName(':focus-within', '', '', '')).toBe('w'); | ||
| expect(getStyleBucketName(':focus', '', '', '')).toBe('f'); | ||
| expect(getStyleBucketName(':focus-visible', '', '', '')).toBe('i'); | ||
| expect(getStyleBucketName(':hover', '', '', '')).toBe('h'); | ||
| expect(getStyleBucketName(':active', '', '', '')).toBe('a'); | ||
| expect(getStyleBucketName([' :link'], '', '', '')).toBe('l'); | ||
| expect(getStyleBucketName([' :hover '], '', '', '')).toBe('h'); | ||
|
|
||
| expect(getStyleBucketName(':active', 'theme', '', '')).toBe('t'); | ||
| expect(getStyleBucketName(':active', '', '(max-width: 100px)', '')).toBe('m'); | ||
| expect(getStyleBucketName(':active', '', '', '(display: table-cell)')).toBe('t'); | ||
| expect(getStyleBucketName([':link'], '', '', '')).toBe('l'); | ||
| expect(getStyleBucketName([':visited'], '', '', '')).toBe('v'); | ||
| expect(getStyleBucketName([':focus-within'], '', '', '')).toBe('w'); | ||
| expect(getStyleBucketName([':focus'], '', '', '')).toBe('f'); | ||
| expect(getStyleBucketName([':focus-visible'], '', '', '')).toBe('i'); | ||
| expect(getStyleBucketName([':hover'], '', '', '')).toBe('h'); | ||
| expect(getStyleBucketName([':active'], '', '', '')).toBe('a'); | ||
|
|
||
| expect(getStyleBucketName([':active'], 'theme', '', '')).toBe('t'); | ||
| expect(getStyleBucketName([':active'], '', '(max-width: 100px)', '')).toBe('m'); | ||
| expect(getStyleBucketName([':active'], '', '', '(display: table-cell)')).toBe('t'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,12 @@ const pseudosMap: Record<string, StyleBucketName | undefined> = { | |
| * | ||
| * @internal | ||
| */ | ||
| export function getStyleBucketName(pseudo: string, layer: string, media: string, support: string): StyleBucketName { | ||
| export function getStyleBucketName( | ||
| selectors: string[], | ||
| layer: string, | ||
| media: string, | ||
| support: string, | ||
| ): StyleBucketName { | ||
| if (media) { | ||
| return 'm'; | ||
| } | ||
|
|
@@ -52,20 +57,22 @@ export function getStyleBucketName(pseudo: string, layer: string, media: string, | |
| return 't'; | ||
| } | ||
|
|
||
| const normalizedPseudo = pseudo.trim(); | ||
| if (selectors.length > 0) { | ||
| const normalizedPseudo = selectors[0].trim(); | ||
|
Comment on lines
+60
to
+61
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We take into account only the first pseudo selector, so it's find to use the first element. |
||
|
|
||
| if (normalizedPseudo.charCodeAt(0) === 58 /* ":" */) { | ||
| // We send through a subset of the string instead of the full pseudo name. | ||
| // For example: | ||
| // - `"focus-visible"` name would instead of `"us-v"`. | ||
| // - `"focus"` name would instead of `"us"`. | ||
| // Return a mapped pseudo else default bucket. | ||
| if (normalizedPseudo.charCodeAt(0) === 58 /* ":" */) { | ||
| // We send through a subset of the string instead of the full pseudo name. | ||
| // For example: | ||
| // - `"focus-visible"` name would instead of `"us-v"`. | ||
| // - `"focus"` name would instead of `"us"`. | ||
| // Return a mapped pseudo else default bucket. | ||
|
|
||
| return ( | ||
| pseudosMap[normalizedPseudo.slice(4, 8)] /* allows to avoid collisions between "focus-visible" & "focus" */ || | ||
| pseudosMap[normalizedPseudo.slice(3, 5)] || | ||
| 'd' | ||
| ); | ||
| return ( | ||
| pseudosMap[normalizedPseudo.slice(4, 8)] /* allows to avoid collisions between "focus-visible" & "focus" */ || | ||
| pseudosMap[normalizedPseudo.slice(3, 5)] || | ||
| 'd' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Return default bucket | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -412,6 +412,21 @@ describe('resolveStyleRules', () => { | |
| padding-right: 10px; | ||
| } | ||
| `); | ||
|
|
||
| expect( | ||
| resolveStyleRules({ | ||
| ':hover,:focus-within': { | ||
| '::before': { | ||
| color: 'orange', | ||
| }, | ||
| }, | ||
| }), | ||
| ).toMatchInlineSnapshot(` | ||
| .fij4gri:hover::before, | ||
| .fij4gri:focus-within::before { | ||
| color: orange; | ||
| } | ||
| `); | ||
|
Comment on lines
+416
to
+429
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test case from #204. |
||
| }); | ||
|
|
||
| it('handles media queries', () => { | ||
|
|
@@ -573,18 +588,6 @@ describe('resolveStyleRules', () => { | |
| `); | ||
| }); | ||
|
|
||
| it('handles :global selector', () => { | ||
| expect( | ||
| resolveStyleRules({ | ||
| ':global(body) &': { color: 'green' }, | ||
| }), | ||
| ).toMatchInlineSnapshot(` | ||
| body .fm1e7ra { | ||
| color: green; | ||
| } | ||
| `); | ||
| }); | ||
|
Comment on lines
-576
to
-586
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again a test that does not have any sense. |
||
|
|
||
| it('handles :global selector', () => { | ||
| expect( | ||
| resolveStyleRules({ | ||
|
|
@@ -609,34 +612,38 @@ describe('resolveStyleRules', () => { | |
| `); | ||
| expect( | ||
| resolveStyleRules({ | ||
| ':global(body):focus': { color: 'pink' }, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now spaces are handled (#206). |
||
| ':global(body) :focus': { color: 'green' }, | ||
| ':global(body) :focus:hover': { color: 'blue' }, | ||
| ':global(body) :focus .foo': { color: 'yellow' }, | ||
| }), | ||
| ).toMatchInlineSnapshot(` | ||
| body .frou13r:focus { | ||
| body .fug6i29:focus { | ||
| color: pink; | ||
| } | ||
| body .frou13r :focus { | ||
| color: green; | ||
| } | ||
| body .f1emv7y1:focus:hover { | ||
| body .f1emv7y1 :focus:hover { | ||
| color: blue; | ||
| } | ||
| body .f1g015sp:focus .foo { | ||
| body .f1g015sp :focus .foo { | ||
| color: yellow; | ||
| } | ||
| `); | ||
| }); | ||
|
|
||
| // it.todo('supports :global as a nested selector', () => { | ||
| // expect( | ||
| // resolveStyleRules({ | ||
| // ':focus': { ':global(body)': { color: 'green' } }, | ||
| // }), | ||
| // ).toMatchInlineSnapshot(` | ||
| // body .fm1e7ra0:focus { | ||
| // color: green; | ||
| // } | ||
| // `); | ||
| // }); | ||
| it('supports :global as a nested selector', () => { | ||
| expect( | ||
| resolveStyleRules({ | ||
| ':focus': { ':global(body)': { color: 'green' } }, | ||
| }), | ||
| ).toMatchInlineSnapshot(` | ||
| body .fz7er5p:focus { | ||
| color: green; | ||
| } | ||
| `); | ||
| }); | ||
|
Comment on lines
+636
to
+646
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As now we have scopes this test surprisingly works 🐱 |
||
| }); | ||
|
|
||
| describe('keyframes', () => { | ||
|
|
||
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.
The original selector is invalid, replaced it with a proper test case.