Introduce primitive color palette; alias duplicated theme tokens#129
Open
theanmolsharma wants to merge 2 commits into
Open
Introduce primitive color palette; alias duplicated theme tokens#129theanmolsharma wants to merge 2 commits into
theanmolsharma wants to merge 2 commits into
Conversation
Introduces a `palette` of raw color values, each declared exactly once, which the semantic tokens in both ShroudDefaultTheme and ShroudDarkTheme now reference. Purely structural — every token resolves to the exact same color as before (verified across all 128 light + 68 dark tokens); no rendered color changes. Addresses the duplication raised in the #124 review: a shared value like #E6E4E4 (previously typed into 4 tokens) or #E6E2FA (6 tokens) now lives in one place, so a palette change happens once. Semantic tokens stay distinct and independently themeable per light/dark — e.g. divider and transactionCardBorder both alias palette.gray200 in light, yet transactionCardBorder still diverges to palette.gray850 in dark. Genuinely unique one-off values are left inline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drops 16 color tokens with zero consumers anywhere in the app, from both ShroudDefaultTheme and ShroudDarkTheme, plus the now-orphaned `blue300` palette primitive (it was referenced only by two of the removed tokens): borderWidth, borderTopColor, buttonBlueBackgroundColor, outgoingBackgroundColor, failedColor, hdbackgroundColor, lnborderColor, lnbackgroundColor, modalButton, feeActive, cta2, outputValue, msSuccessBG, msSuccessCheck, newBlue, secondary Verified dead: no `colors.<token>` access, no destructuring of individual tokens from `colors`, and no dynamic `colors[...]` access anywhere. React Navigation reserved keys (primary/background/…) are retained. `tsc` passes, confirming no consumer referenced any removed token; the values of all remaining tokens are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
i dont like this approach. if a color ends up diverging between dark and light mode, we should fix the token itself instead of introducing a separate color palette. otherwise we're just working around the issue and adding more tokens to maintain. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Followup to the token-duplication discussion in #124 (thread).
That thread surfaced a real tension:
#E6E4E4×4,#E6E2FA×6,#754CE8×11, …), so a palette change means editing the same value in N places.Reusing an existing semantic token (the proposed fix) would have coupled unrelated components; keeping duplicate literals leaves the maintenance debt. The correct fix is the layer we didn't have yet: primitives + semantic aliases.
What
1. Primitive palette (commit 1). Adds a module-level
paletteof raw color values, each declared exactly once. Every semantic token in bothShroudDefaultThemeandShroudDarkThemenow references a palette entry instead of repeating a literal:dividerandtransactionCardBordersharegray200in light, yet the card border still diverges togray850in dark.gray200,violet600), deliberately not tied to any component — as suggested in the thread.incomingIconBackground: '#E7E6F5') stay inline.2. Remove dead tokens (commit 2). Drops 16 color tokens with zero consumers anywhere in the app, plus the now-orphaned
blue300primitive:borderWidth, borderTopColor, buttonBlueBackgroundColor, outgoingBackgroundColor, failedColor, hdbackgroundColor, lnborderColor, lnbackgroundColor, modalButton, feeActive, cta2, outputValue, msSuccessBG, msSuccessCheck, newBlue, secondaryVerified dead: no
colors.<token>access, no destructuring of individual tokens, no dynamiccolors[...]access. React Navigation reserved keys (primary/background/…) are retained.Guarantees
No rendered color changes. Every remaining token resolves byte-identically to its value on
master(verified programmatically across both themes).tsc— no new errors. The removal pass is itself a safety net: had any of the 16 tokens been consumed,tscwould now fail with "Property does not exist on typeTheme". It passes. (Also droppedas conston the palette so token types staystring, preserving theThemetype and the light/dark divergence.)eslint components/themes.ts— clean.Not in scope
Single-use literals were left inline, and token renaming (making semantic names less component-specific) is a separate change, as noted in the thread.
🤖 Generated with Claude Code