-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix stacking variant order when variants inside a group are treated as equal #14431
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 12 commits
d2870dc
ec6edc4
00af756
f111b4b
1f397ec
bdf18ba
d85cb2b
7d77561
20885f0
05d9ee7
9700a1d
00735aa
962590b
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 |
|---|---|---|
|
|
@@ -30,10 +30,7 @@ export function compileCandidates( | |
| matches.set(rawCandidate, candidates) | ||
| } | ||
|
|
||
| // Sort the variants | ||
| let variants = designSystem.getUsedVariants().sort((a, z) => { | ||
| return designSystem.variants.compare(a, z) | ||
| }) | ||
| let variantOrderMap = designSystem.getVariantOrder() | ||
|
|
||
| // Create the AST | ||
| for (let [rawCandidate, candidates] of matches) { | ||
|
|
@@ -51,7 +48,9 @@ export function compileCandidates( | |
| // variants used. | ||
| let variantOrder = 0n | ||
| for (let variant of candidate.variants) { | ||
| variantOrder |= 1n << BigInt(variants.indexOf(variant)) | ||
| let order = variantOrderMap.get(variant) | ||
| if (order === undefined) continue | ||
|
||
| variantOrder |= 1n << BigInt(order) | ||
| } | ||
|
|
||
| nodeSorting.set(node, { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { toCss } from './ast' | ||
| import { parseCandidate, parseVariant, type Candidate } from './candidate' | ||
| import { parseCandidate, parseVariant, type Candidate, type Variant } from './candidate' | ||
| import { compileAstNodes, compileCandidates } from './compile' | ||
| import { getClassList, getVariants, type ClassEntry, type VariantEntry } from './intellisense' | ||
| import { getClassOrder } from './sort' | ||
|
|
@@ -13,17 +13,19 @@ export type DesignSystem = { | |
| utilities: Utilities | ||
| variants: Variants | ||
|
|
||
| candidatesToCss(classes: string[]): (string | null)[] | ||
| getClassOrder(classes: string[]): [string, bigint | null][] | ||
| getClassList(): ClassEntry[] | ||
| getVariants(): VariantEntry[] | ||
|
|
||
| parseCandidate(candidate: string): Candidate[] | ||
| parseVariant(variant: string): ReturnType<typeof parseVariant> | ||
| parseVariant(variant: string): Variant | null | ||
| compileAstNodes(candidate: Candidate): ReturnType<typeof compileAstNodes> | ||
|
|
||
| getUsedVariants(): ReturnType<typeof parseVariant>[] | ||
| getVariantOrder(): Map<Variant, number> | ||
| resolveThemeValue(path: string): string | undefined | ||
|
|
||
| // Used by IntelliSense | ||
| candidatesToCss(classes: string[]): (string | null)[] | ||
| } | ||
|
|
||
| export function buildDesignSystem(theme: Theme): DesignSystem { | ||
|
|
@@ -77,8 +79,29 @@ export function buildDesignSystem(theme: Theme): DesignSystem { | |
| compileAstNodes(candidate: Candidate) { | ||
| return compiledAstNodes.get(candidate) | ||
| }, | ||
| getUsedVariants() { | ||
| return Array.from(parsedVariants.values()) | ||
| getVariantOrder() { | ||
| let variants = Array.from(parsedVariants.values()) | ||
| variants.sort((a, z) => this.variants.compare(a, z)) | ||
|
|
||
| let order = new Map<Variant, number>() | ||
| let prevVariant: Variant | undefined = undefined | ||
| let index: number = 0 | ||
|
|
||
| for (let variant of variants) { | ||
| if (variant === null) { | ||
| continue | ||
| } | ||
| // This variant is not the same order as the previous one | ||
| // so it goes into a new group | ||
| if (prevVariant !== undefined && this.variants.compare(prevVariant, variant) !== 0) { | ||
| index++ | ||
| } | ||
|
|
||
| order.set(variant, index) | ||
| prevVariant = variant | ||
| } | ||
|
|
||
| return order | ||
|
Comment on lines
+82
to
+104
Contributor
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. @thecrypticace came up with this nugget: We can use a
|
||
| }, | ||
|
|
||
| resolveThemeValue(path: `${ThemeKey}` | `${ThemeKey}${string}`) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1265,6 +1265,57 @@ test('sorting stacked min-* and max-* variants', async () => { | |
| `) | ||
| }) | ||
|
|
||
| test('stacked min-* and max-* variants should come after unprefixed variants', async () => { | ||
| expect( | ||
| await compileCss( | ||
| css` | ||
| @theme { | ||
| /* Explicitly ordered in a strange way */ | ||
| --breakpoint-sm: 640px; | ||
| --breakpoint-lg: 1024px; | ||
| --breakpoint-md: 768px; | ||
| } | ||
| @tailwind utilities; | ||
| `, | ||
| ['sm:flex', 'min-sm:max-lg:flex', 'md:flex', 'min-md:max-lg:flex'], | ||
| ), | ||
| ).toMatchInlineSnapshot(` | ||
| ":root { | ||
| --breakpoint-sm: 640px; | ||
| --breakpoint-lg: 1024px; | ||
| --breakpoint-md: 768px; | ||
| } | ||
|
|
||
| @media (width >= 640px) { | ||
| .sm\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
|
|
||
| @media (width >= 640px) { | ||
| @media (width < 1024px) { | ||
| .min-sm\\:max-lg\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @media (width >= 768px) { | ||
| .md\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
|
|
||
| @media (width >= 768px) { | ||
| @media (width < 1024px) { | ||
| .min-md\\:max-lg\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
| }" | ||
|
Comment on lines
+1289
to
+1315
Contributor
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. This now has the non-stacked rules ( |
||
| `) | ||
| }) | ||
|
|
||
| test('min, max and unprefixed breakpoints', async () => { | ||
| expect( | ||
| await compileCss( | ||
|
|
@@ -2246,14 +2297,14 @@ test('container queries', async () => { | |
| --width-lg: 1024px; | ||
| } | ||
|
|
||
| @container (width < 1024px) { | ||
| .\\@max-lg\\:flex { | ||
| @container name (width < 1024px) { | ||
| .\\@max-lg\\/name\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
|
|
||
| @container name (width < 1024px) { | ||
| .\\@max-lg\\/name\\:flex { | ||
| @container (width < 1024px) { | ||
| .\\@max-lg\\:flex { | ||
| display: flex; | ||
| } | ||
|
Comment on lines
+2300
to
2309
Contributor
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. Right now the Previously, both the named and non-named variant would be getting a separate variant order but now they follow through and are ordered based on the raw candidate name I think. |
||
| } | ||
|
|
@@ -2294,20 +2345,14 @@ test('container queries', async () => { | |
| } | ||
| } | ||
|
|
||
| @container (width >= 1024px) { | ||
| .\\@lg\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
|
|
||
| @container name (width >= 1024px) { | ||
| .\\@lg\\/name\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
|
|
||
| @container (width >= 1024px) { | ||
| .\\@min-lg\\:flex { | ||
| .\\@lg\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
|
|
@@ -2316,6 +2361,12 @@ test('container queries', async () => { | |
| .\\@min-lg\\/name\\:flex { | ||
| display: flex; | ||
| } | ||
| } | ||
|
|
||
| @container (width >= 1024px) { | ||
| .\\@min-lg\\:flex { | ||
| display: flex; | ||
| } | ||
| }" | ||
| `) | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,13 +117,22 @@ export class Variants { | |
| if (orderedByVariant !== 0) return orderedByVariant | ||
|
|
||
| if (a.kind === 'compound' && z.kind === 'compound') { | ||
| return this.compare(a.variant, z.variant) | ||
| let order = this.compare(a.variant, z.variant) | ||
| if (order === 0) { | ||
| if (a.modifier && z.modifier) { | ||
| return a.modifier.value < z.modifier.value ? -1 : 1 | ||
| } else if (a.modifier) { | ||
| return 1 | ||
| } else if (z.modifier) { | ||
| return -1 | ||
| } | ||
| } | ||
|
Comment on lines
+120
to
+129
Contributor
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. This now explicitly handles named modifiers for compound variants, so named groups will appear before their equivalent unnamed group. |
||
| return order | ||
| } | ||
|
|
||
| let compareFn = this.compareFns.get(aOrder) | ||
| if (compareFn === undefined) return 0 | ||
|
|
||
| return compareFn(a, z) || (a.root < z.root ? -1 : 1) | ||
| if (compareFn === undefined) return a.root < z.root ? -1 : 1 | ||
| return compareFn(a, z) | ||
| } | ||
|
|
||
| keys() { | ||
|
|
||
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.
These were actually unordered before. If no custom order is provided, we now follow back to ordering the argument if everything else is the same, This will make
bakedappear beforeyellownow.