Handle ringColor.DEFAULT in js configs
#19348
Conversation
WalkthroughAdds handling for Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts (1)
240-246: Migration ofringColor.DEFAULTto--default-ring-colorlooks correctThe upgrade path now emits
--default-ring-colorforringColor.DEFAULTand skips the generickeyPathToCssPropertymapping, keeping the generated CSS consistent with the runtime theme behavior and avoiding an incorrect--ring-colorvariable.If we add more special-cased theme keys in future, consider centralizing this mapping so
apply-config-to-themeand the upgrader can’t drift apart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)integrations/upgrade/js-config.test.ts(2 hunks)packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts(1 hunks)packages/tailwindcss/src/compat/apply-config-to-theme.test.ts(2 hunks)packages/tailwindcss/src/compat/apply-config-to-theme.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts (1)
packages/tailwindcss/src/utils/escape.ts (1)
escape(2-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Linux / upgrade
- GitHub Check: Linux / vite
- GitHub Check: Linux / postcss
- GitHub Check: Linux
🔇 Additional comments (5)
CHANGELOG.md (1)
18-18: Changelog entry correctly describes theringColor.DEFAULTfixThe new “Fixed” bullet is accurate, scoped, and links to the correct PR, so the release notes will clearly communicate this behavior change.
packages/tailwindcss/src/compat/apply-config-to-theme.ts (1)
53-63: Special-case forringColor.DEFAULT→--default-ring-coloris correct and consistentHandling
ringColor.DEFAULTbefore the generickeyPathToCssPropertypath ensures we no longer emit--ring-colorand instead populate--default-ring-colorwith the right theme options, matching the intended v3-compatible behavior.packages/tailwindcss/src/compat/apply-config-to-theme.test.ts (1)
60-63: Tests accurately cover the new default ring color behaviorAdding
ringColor.DEFAULTto the test config and asserting that--ring-coloris unset while--default-ring-coloris'#fff'validates the new mapping end-to-end and guards against regressions.Also applies to: 132-133
integrations/upgrade/js-config.test.ts (2)
37-39: LGTM! Test input properly configured.The
ringColor.DEFAULTconfiguration correctly sets up the test case for validating the v3 to v4 migration behavior. The test value#c0ffeeis clearly identifiable in the output.
197-198: LGTM! Expected output correctly validates the migration.The test properly verifies that
ringColor.DEFAULTmigrates to--default-ring-color(not--ring-color), which aligns with the PR objectives. The snapshot-based assertion implicitly confirms that the old--ring-colorvariable does not appear in the output.
| // ringColor.DEFAULT is a special case that maps to `--default-ring-color` | ||
| // as to match the behavior of v3 | ||
| if (path[0] === 'ringColor' && path[1] === 'DEFAULT') { | ||
| designSystem.theme.add( | ||
| '--default-ring-color', | ||
| '' + value, | ||
| ThemeOptions.INLINE | ThemeOptions.REFERENCE | ThemeOptions.DEFAULT, | ||
| ) | ||
|
|
||
| continue | ||
| } |
There was a problem hiding this comment.
I'm a bit confused why this is handled in isolation here since we had a lot of other DEFAULT special keys that are supported in v4 too via the --default-* pattern, e.g.: --default-ring-width, --default-outline-width, --default-transition-duration. So I'd expect there to a code path a bit more generic to handle all of these variants.
From my quick look it seems that we didn't handle this in apply-config-to-theme at all right now so not sure if this is an oversight or if there's an alternative explanation?
There was a problem hiding this comment.
Yeah this is a good point. We do handle a few default things with regards to fonts but nothing else. Lemme do a bit of poking around to see if we need to handle a few more things.
thecrypticace
force-pushed
the
fix/issue-19345
branch
from
301ef64 to
ebc3f3c
Compare
There’s a few namespaces that need special behavior Handle more default conversions
thecrypticace
force-pushed
the
fix/issue-19345
branch
from
ebc3f3c to
22d3cb2
Compare
Fixes #19345
In v3 the
ringColor.DEFAULToption was used as the default color for ring utilities (when it was defined). This currently gets translated as--ring-colorbut that doesn't work this way in v4. Instead it should translate to--default-ring-colorand not--ring-color.I've also tweaked the upgrade tool to handle this properly as well.