feat: add inset shadow parsing support#277
feat: add inset shadow parsing support#277YevheniiKotyrlo wants to merge 1 commit intonativewind:mainfrom
Conversation
aee4d5d to
4cbd812
Compare
There was a problem hiding this comment.
Thanks for the contribution. The inset shadow parsing is solid. I have concerns about one part though.
-
Inset shadow parsing (
box-shadow.ts) looks good. The new shorthand patterns andnormalizeInsetValuecorrectly handle inset shadows coming through CSS variables at runtime. Statically-written inset shadows are already handled at compile time by lightningcss and the compiler, so this specifically covers the runtime/variable path, which is what matters for Tailwind's generated output. Minor gap:[inset, offsetX, offsetY, blurRadius, color]isn't covered, though Tailwind doesn't generate that pattern so not a blocker. -
Root variable defaults (
root.ts) needs to move. Hardcoding--tw-shadow,--tw-inset-shadow,--tw-ring-shadow, etc. in react-native-css couples the generic CSS engine to Tailwind's internal variable names. react-native-css is framework-agnostic and shouldn't have knowledge of Tailwind-specific variables. These defaults belong in nativewind (e.g., innativewind/theme.cssas:rootvariable declarations). That keeps the separation clean. The architectural intention of v5 is that Nativewind adapts Tailwind for RN while react-native-css handles generic CSS.
Also, rootVariables("tw-shadow-color").set([["initial"]]) seems problematic. What does "initial" resolve to at runtime? It's not a value react-native-css knows how to interpret.
- The inset shadow tests are good. The "Tailwind v4 shadow variables" test should assert more than
.toBeDefined(). Since#0000shadows get filtered byomitTransparentShadows, the test should verify what the resulting array actually contains.
TLDR: Could you split this into two PRs? The inset shadow changes to box-shadow.ts and tests are ready to merge. The root variable defaults need to move to nativewind.
4cbd812 to
aba4514
Compare
|
Thanks for the review. Addressed all three points:
This PR (inset shadow parsing) is ready for re-review — it now only contains the |
5f1cf77 to
a42e31f
Compare
|
@danstepanov Just checking in — this PR now only contains the The minor gap you noted ( Ready for re-review whenever you get a chance. |
|
hey @YevheniiKotyrlo thanks for updating, i'll review this again in the coming days, i'm focused on knocking out some issues in Nativewind v4 atm |
a42e31f to
a3cba9e
Compare
a3cba9e to
0cb0e65
Compare
|
Hi @danstepanov, hope the NativeWind v4 work is going well! No rush at all — just wanted to let you know this one is rebased on latest main and ready whenever you have a moment. Since my last comment I've also opened two more PRs on this repo that might be worth looking at together:
Happy to walk through any of them if that would help. Thanks for maintaining this project! |
|
Thanks @YevheniiKotyrlo reviewing this tomorrow |
|
Sounds great! I've cleaned up the stack — #284 is now based on this branch's commit (2 stacked commits in #284, reviewable individually via the Commits tab). So reviewing this one first is the right order. This PR is minimal: just |
Problem
Tailwind CSS v4 shadow and ring utilities don't work on React Native. Two root causes:
insetkeyword, so shadows arriving through CSS variables at runtime fail to parse. (Fixed here)@propertydefaults — Tailwind v4's@propertydeclarations aren't parsed, so variables like--tw-ring-shadowhave no initial values. (Fixed in feat: add CSS @property rule support #284, stacked on this PR)This is PR 1/2. PR #284 builds directly on this branch.
Changes
src/native/styles/shorthands/box-shadow.tsinsetfield descriptor with literal string matchingnormalizeInsetValue()to convert string"inset"to booleantruefor React Native'sboxShadowpropsrc/__tests__/native/box-shadow.test.tsxReview notes
@propertysupport in feat: add CSS @property rule support #284toStrictEqualwith exact expected values