Support fractional opacity modifiers for named shadow sizes#20302
Conversation
Confidence Score: 5/5This looks safe to merge.
Reviews (2): Last reviewed commit: "update changelog" | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change updates opacity modifier handling in Changes
Sequence Diagram(s)Not applicable. Related PRs: None identified Suggested labels: bug, css Suggested reviewers: philipp-spiess, thecrypticace 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/utilities.ts (1)
4617-4625: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated modifier-alpha resolution.
The exact same
if (arbitrary) ... else if (isValidOpacityValue(...)) ...block is now repeated identically acrossdrop-shadow,text-shadow,shadow, andinset-shadow. This duplication is precisely how the original bug happened — one of the five copies (this one) silently diverged toisPositiveIntegerwhile its siblings kept the correct helper. A small shared helper, e.g.resolveAlphaModifier(candidate.modifier): string | undefined, would remove the risk of this class of drift in the future.♻️ Example helper extraction
+function resolveAlphaModifier(modifier: CandidateModifier | null): string | undefined { + if (!modifier) return undefined + if (modifier.kind === 'arbitrary') return modifier.value + if (isValidOpacityValue(modifier.value)) return `${modifier.value}%` + return undefined +} + utilities.functional('drop-shadow', (candidate) => { - let alpha: string | undefined - - if (candidate.modifier) { - if (candidate.modifier.kind === 'arbitrary') { - alpha = candidate.modifier.value - } else { - if (isValidOpacityValue(candidate.modifier.value)) { - alpha = `${candidate.modifier.value}%` - } - } - } + let alpha = resolveAlphaModifier(candidate.modifier)Same replacement applies to the
text-shadow,shadow, andinset-shadowblocks.Also applies to: 5426-5434, 5572-5580, 5698-5706
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 224abfc2-a8dc-4493-945f-678cf88fd0d8
📒 Files selected for processing (3)
CHANGELOG.mdpackages/tailwindcss/src/utilities.test.tspackages/tailwindcss/src/utilities.ts
Ziiyodullayevv
left a comment
Ziiyodullayevv
left a comment
There was a problem hiding this comment.
Great addition for fractional opacity modifiers on shadow utilities. The implementation:
- The test coverage is comprehensive - testing shadow-sm/12.5, text-shadow-sm/12.5, drop-shadow-sm/12.5, and inset-shadow-sm/12.5
- The inline snapshot tests show the expected CSS output clearly
- The changelog entry is well-written
This brings shadow utilities in line with other opacity modifiers in the codebase. The fix using isValidOpacityValue instead of isPositiveInteger is the right approach.
|
Thanks for the suggestion! Agreed the duplication is exactly how the original bug slipped in. I'd rather keep this PR scoped to just the bug fix though, and leave extracting a shared resolveAlphaModifier helper for a separate follow-up PR — happy to open that if there's interest. |
RobinMalfait
left a comment
RobinMalfait
left a comment
There was a problem hiding this comment.
Thanks for the PR! Made some small changes.
No need to introduce follow up PRs for introducing abstractions 👍
3 participants
Summary
shadow-*,text-shadow-*,drop-shadow-*, andinset-shadow-*accept a bare (non-arbitrary) opacity modifier like/50, but the named-size branch of all four utilities validates it withisPositiveInteger(candidate.modifier.value)instead ofisValidOpacityValue(candidate.modifier.value)— the helper every other opacity/alpha modifier in the codebase uses (asColor, used bybg-*,text-*,border-*,ring-*,fill-*,stroke-*,decoration-*,accent-*,caret-*,outline-*,placeholder-*,divide-*).This means a fractional modifier like
/12.5is silently ignored for a named size (shadow-sm/12.5behaves exactly likeshadow-sm, dropping the modifier), while the exact same/12.5modifier works correctly on the color variant of the same utility (shadow-red-500/12.5→color-mix(in oklab, var(--color-red-500) 12.5%, transparent)), since that path already goes throughasColor.drop-shadow-*is worse: its named-size branch has an extra guard (if (candidate.modifier && !alpha) return) that bails out of the entire utility when a modifier is present but couldn't be resolved to an alpha — sodrop-shadow-sm/12.5produces no CSS at all.This isn't a case of fractional percentages being unsupported by design — the CHANGELOG entry that introduced
shadow-*/<alpha>explicitly describes it as controlling shadow opacity, and the color branch of these same utilities already supports fractional values (shadow-red-500/2.25,/2.5,/2.75are covered by existing tests). The named-size branch just never got the same treatment.Repro (verified with
pnpm --filter tailwindcss exec vitest run):shadow-red-500/12.5→color-mix(in oklab, var(--color-red-500) 12.5%, transparent)(correct)shadow-sm/12.5→ identical output to plainshadow-sm(modifier silently dropped)drop-shadow-sm/12.5→ no CSS generated at allshadow-sm/50(integer, control) → works correctlyFix
Replaced
isPositiveInteger(candidate.modifier.value)withisValidOpacityValue(candidate.modifier.value)in the four affected utility definitions inpackages/tailwindcss/src/utilities.ts(shadow,text-shadow,drop-shadow,inset-shadow). No other changes were needed — oncealpharesolves correctly,drop-shadow's existingif (candidate.modifier && !alpha) returnguard naturally stops bailing out, sincealphais no longerundefinedfor valid fractional modifiers.Test plan
packages/tailwindcss/src/utilities.test.tscoveringshadow-sm/12.5,text-shadow-sm/12.5,drop-shadow-sm/12.5, andinset-shadow-sm/12.5.pnpm --filter tailwindcss exec vitest runthat this test fails (modifier dropped / empty output) with the fix reverted, and passes with it applied.tailwindcsspackage test suite (pnpm --filter tailwindcss exec vitest run) — 4685 tests passing, no regressions.npx prettier --check.