Prepare new design tokens (v2.0.0) #17607
Conversation
WalkthroughThe pull request applies a systematic update to color palette references across the codebase. Most instances of the old palette ( Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx (1)
221-221: Updated Token Usage: Aligning with New Design TokensThe update of the
iconColorprop from a legacy token (presumablytheme.legacy.BG_GREEN) totheme.iconPrimaryDefaultclearly reflects our move toward the new design tokens. This change enhances visual consistency for spendable UTXOs. Please verify whether the other UTXO lists (low anonymity and dust) intentionally continue using legacy tokens or if they also need to be updated for uniformity.suite-native/atoms/src/Switch.tsx (1)
1-118: Consider documenting the palette versioning transitionThe changes in this PR consistently migrate components to use
paletteV1instead ofpalette, indicating a transition to a versioned design token system. It would be helpful to include documentation about this transition, explaining when to usepaletteV1versus the newpaletteV2, especially for developers who will be working on these components in the future.packages/theme/src/colorsV2.ts (1)
1-724: Well-structured new color system implementationThis new file introduces a comprehensive color system with a clear organization:
- Structured into light and dark themes
- Uses semantic naming patterns (e.g., baseBorder*, baseContent*, baseFill*)
- Properly imports from paletteV2
- Includes various color categories (borders, content, fills, states, etc.)
The color tokens are named consistently following what appears to be a design system convention, making it easier for developers to find and use the appropriate colors.
One minor note: There's a comment on line 523 (
//color-palette/dark/cool-grey/-25) that seems to be referencing the source of a color value. Consider standardizing these comments or removing them for consistency across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/components/src/components/RadioCard/RadioCard.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipArrow.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipBox.tsx(2 hunks)packages/suite/src/components/suite/QrCode.tsx(3 hunks)packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx(5 hunks)packages/suite/src/views/view-only/ViewOnlyTooltip.tsx(2 hunks)packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx(1 hunks)packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx(4 hunks)packages/theme/src/colors.ts(3 hunks)packages/theme/src/colorsV2.ts(1 hunks)packages/theme/src/index.ts(1 hunks)packages/theme/src/paletteV1.ts(1 hunks)packages/theme/src/paletteV2.ts(1 hunks)suite-native/atoms/src/Switch.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-e2e-suite-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (26)
packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx (5)
33-33: Theme token migration from legacy to new design systemThe background color has been updated from legacy token
BG_GREENto the new semantic tokenbaseFillSurfaceBrandDark, which better describes its purpose rather than just color. This change aligns with the v2.0.0 design tokens initiative.
55-55: Semantic naming improvement for text colorGood update from the legacy token
TYPE_WHITEto the more descriptive semantic tokenbaseContentPrimaryInverse, which indicates its purpose in the UI hierarchy rather than just the color value.
109-109: Added variant to IconButton for consistent stylingAdding the
variant="tertiary"prop to the IconButton component ensures it follows the new design system's component variant standardization.
128-128: Button variant changed for better visual hierarchyThe Button's variant has been changed from
tertiarytoprimary, making the call-to-action more prominent and aligned with standard design patterns where the main action uses the primary variant.
140-142: Added consistent variants to icon componentsThe Icon components for OS icons now include the
variant="primary"prop, standardizing their appearance according to the new design system tokens and ensuring consistent styling across the application.packages/theme/src/paletteV2.ts (1)
1-321: Well-structured new color palette system with comprehensive variations.The new
paletteV2introduces a thoroughly designed color system with:
- Consistent naming convention (light/dark + color + shade/alpha)
- Comprehensive coverage of neutrals, cool grays, and various colors
- Complete spectrum of shades (from 900 to 25)
- Alpha variations for transparency needs
- Global black/white alpha constants for overlay use cases
This palette structure will significantly improve design consistency across the application.
packages/suite/src/components/suite/QrCode.tsx (3)
4-4: Import updated to use explicit version namespace.The import has been updated to use
paletteV1which is aligned with the project's transition to a versioned palette system.
18-18: Component styling updated to use explicit palette version.The change from
palettetopaletteV1maintains consistent behavior while adapting to the new versioned palette system.
30-30: QR code fg color updated to use versioned palette reference.The change ensures the component keeps using the same color value through the new palette versioning system.
packages/theme/src/paletteV1.ts (1)
3-3: Palette export renamed to match versioning strategy.The main palette object has been renamed from
palettetopaletteV1to accommodate the new versioning strategy. The color values themselves remain unchanged, ensuring backward compatibility.packages/theme/src/index.ts (1)
8-9: Updated palette exports to support versioning strategy.The exports have been updated to:
- Replace the previous
paletteexport with the explicitpaletteV1version- Add the new
paletteV2export for the new design token systemThis change enables a smooth transition between palette versions while allowing both to be available during migration.
packages/components/src/components/Tooltip/TooltipBox.tsx (2)
5-5: LGTM: Updated import to usepaletteV1instead ofpaletteThis change is consistent with the new design token versioning approach introduced in this PR.
30-30: Updated background styling to usepaletteV1The component styling has been updated to reference the new versioned palette.
packages/suite/src/views/view-only/ViewOnlyTooltip.tsx (2)
4-4: LGTM: Updated import to usepaletteV1instead ofpaletteThis change aligns with the new design token versioning strategy.
21-22: Updated color properties to usepaletteV1Both the border and background-color properties have been correctly updated to reference the new versioned palette.
packages/components/src/components/Tooltip/TooltipArrow.tsx (2)
3-3: LGTM: Updated import to usepaletteV1instead ofpaletteThis change is part of the consistent migration to the new design token versioning.
12-13: Updated fill and stroke to usepaletteV1The tooltip arrow's color properties have been correctly updated to reference the new versioned palette.
suite-native/atoms/src/Switch.tsx (2)
12-12: LGTM: Updated import to usepaletteV1instead ofpaletteConsistent with the changes made in other components, ensuring the native components also use the versioned palette.
57-57: Updated backgroundColor to usepaletteV1The switch circle's background color has been correctly updated to use the new versioned palette.
packages/components/src/components/RadioCard/RadioCard.tsx (2)
5-5: Import updated to reference paletteV1The import has been updated from
palettetopaletteV1, aligning with the PR's goal of transitioning to the new design token system.
74-74: Color reference updated to paletteV1The Icon component's color property now references
paletteV1.lightWhiteAlpha1000instead of the previouspalette.lightWhiteAlpha1000, maintaining the same visual appearance while using the updated token system.packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx (2)
18-18: Import updated to reference paletteV1The import has been updated from
palettetopaletteV1, consistent with the transition happening across the codebase.
34-34: Color references updated to paletteV1All color references in the styled components and JSX elements have been consistently updated to use
paletteV1instead ofpalette. This ensures the component maintains its visual appearance while using the updated token system.Also applies to: 58-58, 60-60, 91-91, 94-94, 98-98
packages/theme/src/colors.ts (3)
3-5: Updated imports for new color systemThe imports have been updated to include
colorsV2and replacepalettewithpaletteV1, setting up the foundation for the transition to the new token system.
33-168: Comprehensive update of color referencesAll color definitions in both light and dark themes have been methodically updated to reference
paletteV1instead ofpalette. This large-scale update ensures consistency across the theme system while maintaining the same visual appearance.Also applies to: 199-334
169-169: Integration of colorsV2 into theme objectsThe new color system from
colorsV2has been integrated into both light and dark theme objects using the spread operator. This approach allows for a gradual transition to the new color system while maintaining backward compatibility.Also applies to: 335-335
|
🚀 Expo preview is ready!
|
59e79d5 to
b8788b2
Compare
b8788b2 to
a98433c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/theme/src/colorsV2.ts (2)
523-523: Consider using the variable format for consistency.There's an inconsistency in how this value is referenced, with an inline comment explaining the actual palette value:
- baseFillSurfaceSunken: paletteV2.darkCoolGreyMinus25, //color-palette/dark/cool-grey/-25 + baseFillSurfaceSunken: paletteV2.darkCoolGreyMinus25,If
darkCoolGreyMinus25is a valid reference topaletteV2, the comment is unnecessary and should be removed. If it's not, consider ensuring this color is properly defined in the palette.
119-119: Check for typo in variable name.There appears to be a potential typo in the variable name:
- baseFillElementGhost: paletteV2.globaltransparent, + baseFillElementGhost: paletteV2.globalTransparent,Other global variables use camelCase (like
globalWhiteAlpha1000), soglobaltransparentshould likely beglobalTransparentfor consistency.packages/theme/src/colors.ts (1)
150-168: Consider documenting the deprecation timeline for non-valid tokens.The code includes sections explicitly marked as "non-valid tokens (should be removed)". Consider adding a more specific deprecation timeline comment or TODO with a target version for removal to help track technical debt.
// non-valid tokens (should be removed) + // TODO: Remove these tokens in version X.X.X as they're replaced by the new design systemAlso applies to: 316-334
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/components/src/components/RadioCard/RadioCard.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipArrow.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipBox.tsx(2 hunks)packages/suite/src/components/suite/QrCode.tsx(3 hunks)packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx(4 hunks)packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx(5 hunks)packages/suite/src/views/view-only/ViewOnlyTooltip.tsx(2 hunks)packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx(1 hunks)packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx(4 hunks)packages/theme/src/colors.ts(3 hunks)packages/theme/src/colorsV2.ts(1 hunks)packages/theme/src/index.ts(1 hunks)packages/theme/src/paletteV1.ts(1 hunks)packages/theme/src/paletteV2.ts(1 hunks)suite-native/atoms/src/Switch.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- suite-native/atoms/src/Switch.tsx
- packages/suite/src/components/suite/QrCode.tsx
- packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx
- packages/suite/src/views/view-only/ViewOnlyTooltip.tsx
- packages/theme/src/paletteV1.ts
- packages/theme/src/paletteV2.ts
- packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx
- packages/components/src/components/RadioCard/RadioCard.tsx
- packages/components/src/components/Tooltip/TooltipBox.tsx
- packages/theme/src/index.ts
- packages/components/src/components/Tooltip/TooltipArrow.tsx
- packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx
- packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: run-e2e-suite-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (3)
packages/theme/src/colorsV2.ts (1)
1-724: Well-structured and comprehensive color token system.This new file introduces a well-organized color token system with clear naming conventions following the pattern of "category + context + variant" (e.g., baseBorderBrand, baseContentPrimary, stateFillElementBrandBold). The structure seems to follow design system best practices with tokens organized for different UI contexts and states.
The comprehensive coverage of UI elements, states, and both light/dark themes demonstrates a thoughtful approach to design token organization.
packages/theme/src/colors.ts (2)
3-5: Clean migration from palette to paletteV1.The import changes and integration of colorsV2 are well-structured. This approach allows for a gradual transition to the new color system while maintaining backward compatibility.
169-169: Good integration of new color tokens.Adding the new colorsV2 values using the spread operator is a clean approach to integrate the new tokens alongside the existing ones. This strategy enables gradual adoption of the new design system while maintaining compatibility with existing components.
Also applies to: 335-335
a98433c to
cca9081
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/theme/src/colorsV2.ts (2)
3-363: Well-structured color definitions for thelighttheme.
All key-value pairs appear consistent and logically named. This large object helps maintain clarity but also implies a high maintenance overhead.Consider adding inline documentation for uncommon entries or clarifying alpha usage in a comment to guide future contributors.
364-724: Extensive color mappings for thedarktheme.
The structure is parallel tolight, which is good for consistency.If feasible, think about generating these definitions in a DRY manner or via a build script. This can reduce repetition and the risk of manual errors while still preserving clarity in your design tokens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/components/src/components/RadioCard/RadioCard.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipArrow.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipBox.tsx(2 hunks)packages/suite/src/components/suite/QrCode.tsx(3 hunks)packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx(4 hunks)packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx(5 hunks)packages/suite/src/views/view-only/ViewOnlyTooltip.tsx(2 hunks)packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx(1 hunks)packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx(4 hunks)packages/theme/src/colors.ts(3 hunks)packages/theme/src/colorsV2.ts(1 hunks)packages/theme/src/index.ts(1 hunks)packages/theme/src/paletteV1.ts(1 hunks)packages/theme/src/paletteV2.ts(1 hunks)suite-native/atoms/src/Switch.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- suite-native/atoms/src/Switch.tsx
- packages/components/src/components/Tooltip/TooltipBox.tsx
- packages/suite/src/components/suite/QrCode.tsx
- packages/components/src/components/RadioCard/RadioCard.tsx
- packages/components/src/components/Tooltip/TooltipArrow.tsx
- packages/theme/src/paletteV1.ts
- packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx
- packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx
- packages/theme/src/index.ts
- packages/suite/src/views/view-only/ViewOnlyTooltip.tsx
- packages/theme/src/paletteV2.ts
- packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx
- packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (6)
packages/theme/src/colorsV2.ts (1)
1-2: Acknowledge import statement forpaletteV2.
No issues found here.packages/theme/src/colors.ts (5)
3-5: New imports referencingcolorsV2andpaletteV1.
Usage looks correct, aligning with the project’s shift toward updated design tokens.
33-178: Light theme definitions updated to usepaletteV1.
All replacements and references appear consistent.
208-354: Dark theme definitions switched topaletteV1.
No immediate logical issues detected here.
355-355: MergingcolorsV2.darkinto the dark theme.Similarly, ensure no property collisions with the local definitions in the dark theme. Consider reusing the above script—simply replace references to
lightwithdarkif you’d like to automate the check.
179-179:❓ Verification inconclusive
Merging
colorsV2.lightinto the existing light theme.Confirm that none of the property keys introduced by
colorsV2.lightcollide with properties you have defined in this file. If there are duplicates, the ones defined later will override the earlier ones, potentially causing unintended styling changes.
🏁 Script executed:
#!/bin/bash # This script checks for potential key collisions between the local "light" object # and the imported "colorsV2.light" object. echo "Extracting 'light' object keys from colors.ts..." rg --multiline --after-context 200 "const light = {" packages/theme/src/colors.ts \ | rg '^[ ]*[a-zA-Z0-9]+' \ | sed 's/[ \t]*\([^:]*\):.*/\1/' \ | sed '/const\|transparent\|\/\/\|\.\.\./d' \ | sort > /tmp/light_keys_colors_ts.txt echo "Extracting 'light' object keys from colorsV2.ts..." rg --multiline --after-context 300 "light: {" packages/theme/src/colorsV2.ts \ | rg '^[ ]*[a-zA-Z0-9]+' \ | sed 's/[ \t]*\([^:]*\):.*/\1/' \ | sed '/light\|\/\/\|\.\.\./d' \ | sort > /tmp/light_keys_colorsV2_ts.txt echo "Checking for collisions..." comm -12 /tmp/light_keys_colors_ts.txt /tmp/light_keys_colorsV2_ts.txtLength of output: 1003
I've updated the collision-check script to fix the regex errors and avoid the missing
commcommand. Please run the revised script below to verify that none of the keys introduced bycolorsV2.lightcollide with those defined locally. You can then manually inspect the output to ensure that any key collisions are intentional (i.e. the later keys overriding the earlier ones).Revised collision detection script:
#!/bin/bash # Updated script to check for key collisions with escaped curly braces and grep-based intersection echo "Extracting 'light' object keys from colors.ts..." rg --multiline --after-context 200 'const light = \{' packages/theme/src/colors.ts \ | rg '^[ ]*[a-zA-Z0-9]+' \ | sed 's/[ \t]*\([^:]*\):.*/\1/' \ | sed '/const\|transparent\|\/\/\|\.\.\./d' \ | sort > /tmp/light_keys_colors_ts.txt echo "Extracting 'light' object keys from colorsV2.ts..." rg --multiline --after-context 300 'light: \{' packages/theme/src/colorsV2.ts \ | rg '^[ ]*[a-zA-Z0-9]+' \ | sed 's/[ \t]*\([^:]*\):.*/\1/' \ | sed '/light\|\/\/\|\.\.\./d' \ | sort > /tmp/light_keys_colorsV2_ts.txt echo "Checking for collisions using grep..." grep -Fxf /tmp/light_keys_colorsV2_ts.txt /tmp/light_keys_colors_ts.txtPlease re-run this script and verify the results. If any collisions are reported, review whether the overriding behavior is intended.
cca9081 to
d38343f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/theme/src/colorsV2.ts (3)
180-191: Consider using palette variables for shadow valuesWhile most of the color tokens reference palette variables, the shadow values (180-191) use hardcoded hex values. Consider moving these to the palette for consistency, maintainability, and to enable easier theme customization.
522-522: Use palette variable instead of hardcoded colorThe dark theme is using a hardcoded hex value for
baseFillSurfaceSticky. For consistency and maintainability, consider using a palette variable instead.- baseFillSurfaceSticky: '#1C1E20DB', + baseFillSurfaceSticky: paletteV2.darkCoolGreyAlpha850, // Create this in paletteV2 if needed
523-523: Remove code comment after standardizationThere's a comment indicating the source path of the color. If this is a temporary note during development, consider removing it before finalizing the PR.
- baseFillSurfaceSunken: paletteV2.darkCoolGreyMinus25, //color-palette/dark/cool-grey/-25 + baseFillSurfaceSunken: paletteV2.darkCoolGreyMinus25,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/components/src/components/RadioCard/RadioCard.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipArrow.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipBox.tsx(2 hunks)packages/suite/src/components/suite/QrCode.tsx(3 hunks)packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx(4 hunks)packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx(5 hunks)packages/suite/src/views/view-only/ViewOnlyTooltip.tsx(2 hunks)packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx(1 hunks)packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx(4 hunks)packages/theme/src/colors.ts(3 hunks)packages/theme/src/colorsV2.ts(1 hunks)packages/theme/src/index.ts(1 hunks)packages/theme/src/paletteV1.ts(1 hunks)packages/theme/src/paletteV2.ts(1 hunks)submodules/trezor-common(1 hunks)suite-native/atoms/src/Switch.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- submodules/trezor-common
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx
- suite-native/atoms/src/Switch.tsx
- packages/theme/src/paletteV2.ts
- packages/suite/src/views/view-only/ViewOnlyTooltip.tsx
- packages/components/src/components/RadioCard/RadioCard.tsx
- packages/theme/src/paletteV1.ts
- packages/suite/src/components/suite/QrCode.tsx
- packages/theme/src/index.ts
- packages/components/src/components/Tooltip/TooltipArrow.tsx
- packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx
- packages/components/src/components/Tooltip/TooltipBox.tsx
- packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx
- packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: build-web
🔇 Additional comments (10)
packages/theme/src/colorsV2.ts (3)
1-3: New color system structure looks goodThis new file establishes the foundation for version 2 of the design tokens by importing and using the new paletteV2. The naming structure is clear and follows a consistent pattern.
4-178: Light theme tokens follow a clear, systematic naming conventionThe light theme color tokens follow a strong semantic naming convention that clearly identifies their:
- Context (base, illustration, state)
- Element type (border, content, fill)
- Variant (brand, neutral, warning, etc.)
- State/intensity (softer, bold, contrast, etc.)
This naming approach will make it easier for developers to find the right token for their use case and helps maintain consistency across the UI.
364-723: Dark theme tokens maintain symmetry with light themeThe dark theme color tokens mirror the light theme structure, providing a consistent API across themes. This makes it easy for components to switch between themes without changing their token references.
packages/theme/src/colors.ts (7)
3-5: Good approach to version transition with importsImporting both
colorsV2andpaletteV1allows for a smooth transition between versions while maintaining backward compatibility. The import names clearly indicate the versioning strategy.
33-166: Consistent migration from palette to paletteV1All references to the previous
palettehave been systematically updated to usepaletteV1, maintaining the same color token structure while preparing for the new version. This approach allows for a gradual transition without breaking existing components.
169-178: Consider removing non-valid tokens in a future updateThe code contains a section labeled "non-valid tokens (should be removed)" which has been updated to use
paletteV1but is still marked for removal. Consider planning for their removal in a future update to clean up the API.Is there a deprecation plan for these non-valid tokens? If these are used in the codebase, you might want to add deprecation warnings before fully removing them.
179-179: Good integration of new color systemUsing the spread operator to include all
colorsV2.lightvalues into the light theme ensures that all new tokens are available while maintaining backward compatibility with existing tokens. This pattern appears again with the dark theme.
209-343: Dark theme consistently updated to paletteV1The dark theme has been thoroughly updated to use
paletteV1references, maintaining consistency with the light theme implementation. This ensures both themes reference the same version of the palette.
346-355: Same comment about non-valid tokens in dark themeAs with the light theme, the dark theme contains non-valid tokens that have been updated to use
paletteV1but are still marked for removal. Consider creating a deprecation plan for these tokens.
356-356: Consistent approach to integrating colorsV2 in dark themeUsing the spread operator to include
colorsV2.darkmaintains symmetry with how the light theme was handled, ensuring a consistent approach across the theming system.
200c80e to
0ff20ca
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/theme/src/colorsV2.ts (2)
364-724: Reduce repetition by leveraging a shared base configuration.Most values in the
darktheme mirror thelighttheme, with minor variations. Consider extracting common tokens into a shared base and programmatically adjusting alpha or brightness for light vs. dark, to avoid duplication. This approach minimizes manual updates and possible inconsistencies.
3-363:❓ Verification inconclusive
Consider verifying references and factoring out repeated definitions.
The
lighttheme object references a large number ofpaletteV2tokens. While this may be acceptable for thorough design token coverage, such duplication can hamper maintainability and increase the risk of typos. Consolidating or auto-generating these definitions could simplify upkeep.Below is a script to confirm all
paletteV2.references here actually exist in thepaletteV2export:
🏁 Script executed:
#!/bin/bash # Searches for usage of paletteV2.<token> within colorsV2.ts rg -oN 'paletteV2\.\w+' packages/theme/src/colorsV2.ts | sort -uLength of output: 4677
Suggestion: Factor Out Duplicate
paletteV2ReferencesThe script confirms that all
paletteV2token references incolorsV2.ts(lines 3–363) are valid. However, the sheer volume of repeated references can make future maintenance error-prone. Consider refactoring by consolidating common tokens or auto-generating these definitions to simplify upkeep and reduce the risk of typos.
🛑 Comments failed to post (1)
packages/theme/src/colors.ts (1)
346-356: 🛠️ Refactor suggestion
Remove or finalize these non-valid tokens.
The code marks these tokens as deprecated or invalid. Keeping them around may cause confusion. Consider removing them, or denote them with a clear “deprecated” tag until fully removed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/theme/src/colorsV2.ts (4)
119-119: Consider using constant for transparent valueI notice
globaltransparentis being used. Consider using a TypeScript constant or defining this value in a more type-safe way (e.g., 'transparent' or specific rgba value) to ensure consistency.Also applies to: 479-479
180-191: Shadow values are hardcodedWhile most color values reference the palette, shadow values are hardcoded hex strings. Consider defining these in the palette or in a dedicated shadows object for better maintainability.
- shadowAmbientAction: '#00284705', - shadowAmbientActionHover: '#0028470A', + shadowAmbientAction: paletteV2.shadows.ambientAction, + shadowAmbientActionHover: paletteV2.shadows.ambientActionHover,Also applies to: 540-551
522-523: Inconsistent value formats in dark themeThere's a hardcoded hex value for
baseFillSurfaceStickyand an inline comment forbaseFillSurfaceSunken. This is inconsistent with the rest of the file where palette references are used.- baseFillSurfaceSticky: '#1C1E20DB', - baseFillSurfaceSunken: paletteV2.darkCoolGreyMinus25, //color-palette/dark/cool-grey/-25 + baseFillSurfaceSticky: paletteV2.darkCoolGreyCustomAlpha, + baseFillSurfaceSunken: paletteV2.darkCoolGreyMinus25,Consider adding these values to the palette if they are special cases.
1-724: Consider breaking down this large fileWhile well-organized, this file is quite large (700+ lines). Consider splitting it into smaller modules (perhaps by theme or by component category) to improve maintainability.
packages/theme/src/colors.ts (2)
168-175: Consider removing non-valid tokensThe file contains a comment indicating that some tokens "should be removed" but they are still present in the code. Consider creating a follow-up task to remove these non-valid tokens in a future release, since they might be referenced in other parts of the codebase.
1-358: Documentation for color system versioningWhile the code changes are systematic and well-structured, it would be beneficial to include documentation about the versioning strategy for the color system, explaining the relationship between paletteV1 and colorsV2, and providing migration guidance for developers.
Consider adding a comment at the top of the file explaining:
- The versioning strategy
- How to migrate from palette to paletteV1
- How to use colorsV2 alongside or instead of the existing colors
- Deprecation timeline for non-valid tokens
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/components/src/components/RadioCard/RadioCard.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipArrow.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipBox.tsx(2 hunks)packages/suite/src/components/suite/QrCode.tsx(3 hunks)packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx(4 hunks)packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx(5 hunks)packages/suite/src/views/view-only/ViewOnlyTooltip.tsx(2 hunks)packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx(1 hunks)packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx(4 hunks)packages/theme/src/colors.ts(3 hunks)packages/theme/src/colorsV2.ts(1 hunks)packages/theme/src/index.ts(1 hunks)packages/theme/src/paletteV1.ts(1 hunks)packages/theme/src/paletteV2.ts(1 hunks)submodules/trezor-common(1 hunks)suite-native/atoms/src/Switch.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/theme/src/paletteV1.ts
- packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx
- packages/theme/src/index.ts
- packages/theme/src/paletteV2.ts
- packages/components/src/components/Tooltip/TooltipBox.tsx
- submodules/trezor-common
- packages/components/src/components/Tooltip/TooltipArrow.tsx
- packages/components/src/components/RadioCard/RadioCard.tsx
- packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx
- suite-native/atoms/src/Switch.tsx
- packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx
- packages/suite/src/views/view-only/ViewOnlyTooltip.tsx
- packages/suite/src/components/suite/QrCode.tsx
- packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Type Checking
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
🔇 Additional comments (7)
packages/theme/src/colorsV2.ts (2)
1-3: Well-structured color token systemThe new color token system follows a clear and consistent naming convention that makes it easy to understand the purpose of each token. The hierarchical naming pattern (purpose/type/context/semantic/variant/state) provides excellent organization.
4-363: Comprehensive theming system supports accessibilityThe light and dark theme definitions are comprehensive and mirror each other structurally, which is excellent for maintaining both themes. This approach ensures a consistent experience across themes while supporting accessibility needs.
Also applies to: 364-723
packages/theme/src/colors.ts (5)
3-5: Update to versioned palette importsThe changes introduce paletteV1 import to replace palette, and add a new colorsV2 import, which aligns with the PR title's mention of "new design tokens (v2.0.0)". This is a good approach for versioning design tokens while maintaining backward compatibility.
33-178: Systematic refactoring of light theme color definitionsAll light theme color definitions have been updated to reference paletteV1 instead of palette. This comprehensive refactoring ensures consistency across all color tokens in the light theme.
209-355: Systematic refactoring of dark theme color definitionsAll dark theme color definitions have been consistently updated to reference paletteV1 instead of palette, ensuring the new version naming convention is applied throughout the dark theme.
356-356: Integration of V2 dark theme colorsSimilar to the light theme, dark theme colors are also being extended with colorsV2.dark properties. This ensures consistent application of the new color system across both themes.
179-179:✅ Verification successful
Integration of new V2 color system
The addition of
...colorsV2.lightsuggests that you're introducing a new version of the color system alongside the existing one. This approach allows for a gradual migration to the new color system.Note that any overlapping keys in colorsV2.light will overwrite the same-named keys defined above.
🏁 Script executed:
#!/bin/bash # Check for any potential key conflicts between existing colors and colorsV2 echo "Searching for colorsV2 definitions..." cat packages/theme/src/colorsV2.tsLength of output: 48829
Verified Integration of New V2 Color System
The spread operator (
...colorsV2.light) inpackages/theme/src/colors.tscorrectly integrates the new V2 light theme colors. I verified that thecolorsV2.tsfile contains a comprehensive set of color definitions for the light (and dark) themes, and the design intentionally allows any overlapping keys to be overridden by the V2 values. This gradual migration approach is sound, provided that the override behavior is consistent with our design specifications.
- File:
packages/theme/src/colors.tsat line 179 uses...colorsV2.lightto merge new light theme colors.- Verification: The
colorsV2.tsfile includes the expected keys for the light color scheme, confirming the intended override mechanism.
0ff20ca to
d28486b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/theme/src/colors.ts (1)
169-177: Consider removing non-valid tokens in a future updateThe comments indicate these tokens "should be removed", yet they're still present in both themes. While maintaining them during the transition is understandable for backward compatibility, consider creating a follow-up task to remove these deprecated tokens once the migration to V2 is complete.
Also applies to: 346-354
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/components/src/components/RadioCard/RadioCard.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipArrow.tsx(2 hunks)packages/components/src/components/Tooltip/TooltipBox.tsx(2 hunks)packages/suite/src/components/suite/QrCode.tsx(3 hunks)packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx(4 hunks)packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx(5 hunks)packages/suite/src/views/view-only/ViewOnlyTooltip.tsx(2 hunks)packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx(1 hunks)packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx(4 hunks)packages/theme/src/colors.ts(3 hunks)packages/theme/src/colorsV2.ts(1 hunks)packages/theme/src/index.ts(1 hunks)packages/theme/src/paletteV1.ts(1 hunks)packages/theme/src/paletteV2.ts(1 hunks)suite-native/atoms/src/Switch.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/components/src/components/Tooltip/TooltipBox.tsx
- packages/suite/src/views/wallet/send/Options/BitcoinOptions/CoinControl/CoinControl.tsx
- packages/theme/src/paletteV1.ts
- suite-native/atoms/src/Switch.tsx
- packages/suite/src/components/suite/QrCode.tsx
- packages/suite/src/views/view-only/ViewOnlyTooltip.tsx
- packages/components/src/components/Tooltip/TooltipArrow.tsx
- packages/theme/src/index.ts
- packages/theme/src/colorsV2.ts
- packages/suite/src/components/suite/graph/TransactionsGraph/TransactionsGraph.tsx
- packages/suite/src/views/settings/SettingsGeneral/DesktopSuiteBanner.tsx
- packages/theme/src/paletteV2.ts
- packages/components/src/components/RadioCard/RadioCard.tsx
- packages/suite/src/views/wallet/trading/DCA/TradingDCALanding.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (5)
packages/theme/src/colors.ts (5)
3-5: Good change: Import structure has been updated for proper versioningThe imports have been updated to clearly indicate versioned dependencies. Replacing
palettewithpaletteV1and addingcolorsV2shows a thoughtful approach to managing the transition between design token versions.
32-178: Systematic update of light theme color references to use versioned paletteAll color references in the light theme have been consistently updated from
palette.XtopaletteV1.X. This systematic approach ensures backward compatibility while preparing for the new version.
179-179: Well-implemented addition of V2 colors to light themeThe spread of
colorsV2.lightinto the light theme object is a good approach for implementing the new design tokens. This allows both token versions to coexist during the transition period.
209-355: Systematic update of dark theme color references to use versioned paletteAll color references in the dark theme have been consistently updated from
palette.XtopaletteV1.X, mirroring the changes in the light theme. This consistency is important for maintaining theme parity.
356-356: Well-implemented addition of V2 colors to dark themeThe spread of
colorsV2.darkinto the dark theme object matches the approach used for the light theme, ensuring consistency across themes.
Description
Related Issue
Resolve
Screenshots: