feat: added style options #552
Conversation
WalkthroughThis pull request extends the theme system to support modal styling configuration and enhances button component styling. Changes include replacing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
Files:
⏰ 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)
🔇 Additional comments (8)
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/vechain-kit/src/theme/tokens.ts (2)
28-39: Regex may not match all valid CSS size formats.The regex
^\d+(\.\d+)?(rem|px|%|em|vw|vh|ch|ex)$/irequires a numeric prefix, which misses valid CSS values likecalc(...), CSS variables (var(--width)), and unitless0. Consider expanding to handle these cases or pass through unrecognized patterns as-is (which the fallback already does).Current fallback behavior returns the original value if not matched, which is acceptable. However, the comment suggests it validates CSS sizes, which may mislead developers.
function convertChakraSizeToRem(size: string | undefined): string | undefined { if (!size) return undefined; - // If it's already a valid CSS size (contains rem, px, %, em, vw, vh, etc.), return as-is - if (/^\d+(\.\d+)?(rem|px|%|em|vw|vh|ch|ex)$/i.test(size.trim())) { + // If it looks like a CSS length value (number with unit, calc, var, etc.), return as-is + const trimmed = size.trim(); + if (/^\d+(\.\d+)?(rem|px|%|em|vw|vh|ch|ex)$/i.test(trimmed) || + trimmed.startsWith('calc(') || + trimmed.startsWith('var(') || + trimmed === '0') { return size; }
162-166: Consider using JSDoc@deprecatedfor better IDE support.The deprecation notice in the inline comment won't trigger IDE warnings. Adding a proper JSDoc annotation would improve developer experience.
modal?: { backgroundColor?: string; // Base background color for modal (used to derive card, stickyHeader, etc. via opacity) border?: string; // Full CSS border string for modal dialog (e.g., "1px solid rgba(255, 255, 255, 0.1)") backdropFilter?: string; // Backdrop filter for modal dialog (e.g., "blur(10px)") width?: string; // Modal dialog width (e.g., "22rem", "400px") + /** @deprecated Use `rounded` instead */ borderRadius?: string; // Modal dialog border radius (e.g., "24px", "1rem") - deprecated, use rounded instead rounded?: string | number; // Border radius (Chakra UI rounded prop: "sm", "md", "lg", "xl", "2xl", "3xl", "full", or number) };packages/vechain-kit/src/components/common/BaseModal.tsx (1)
43-55: DuplicateuseVechainKitThemeConfig()call.The hook is called twice (lines 43 and 50). Consolidate into a single destructuring call for cleaner code and to avoid redundant hook invocations.
const [isDesktop] = useMediaQuery('(min-width: 768px)'); - const { portalRootRef } = useVechainKitThemeConfig(); + const { portalRootRef, tokens } = useVechainKitThemeConfig(); // Use semantic tokens for modal and overlay colors const modalBg = useToken('colors', 'vechain-kit-modal'); const overlayBg = useToken('colors', 'vechain-kit-overlay'); // Get backdrop filter and modal width from tokens context - const { tokens } = useVechainKitThemeConfig(); const defaultBackdropFilter = tokens?.effects?.backdropFilter?.overlay;packages/vechain-kit/src/utils/cssVariables.ts (2)
55-65: Token-driven modal width for DAppKit looks good; consider a defensive fallbackUsing
tokens.sizes.modalfor--vdk-modal-widthis a nice move toward fully tokenized sizing and keeps DAppKit aligned with the theme system.If there’s any chance a consumer can omit
sizes.modalin a custom theme, you may want a small fallback to avoidundefinedleaking into CSS:- '--vdk-modal-width': tokens.sizes.modal, + '--vdk-modal-width': tokens.sizes.modal ?? '22rem',If
ThemeTokensguaranteessizes.modalis always set, then the current version is fine.
273-325: Privy modalmodalWidthwiring is sound; minor behavior nuance worth notingThe new
modalWidth?: stringparameter and conditional CSS block correctly:
- Keep the function signature backward compatible (optional param added last).
- Scope width/max-width only to the content element (
[data-privy-dialog-content], .privy-dialog-content).- Avoid injecting CSS when
modalWidthis falsy.Two optional considerations:
Style clearing semantics – If
applyPrivyCSSVariablesis later called withoutmodalWidth(and without other style args),cssRules.lengthwill be0, sostyleElement.textContentis not updated and old width rules remain. That’s pre-existing behavior for other options, but now it also affects modal width; if you ever need to “reset” to Privy defaults, you might want:if (cssRules.length > 0) { styleElement.textContent = cssRules.join('\n');
- } else {
styleElement.textContent = '';
- Consistency with DAppKit – Ensure the caller passes the same source (e.g.,
tokens.sizes.modal) here so Privy and DAppKit modals stay visually aligned in width.Otherwise, the implementation looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/vechain-kit/src/components/common/BaseModal.tsx(1 hunks)packages/vechain-kit/src/providers/VeChainKitProvider.tsx(1 hunks)packages/vechain-kit/src/theme/button.ts(4 hunks)packages/vechain-kit/src/theme/modal.ts(1 hunks)packages/vechain-kit/src/theme/tokens.ts(16 hunks)packages/vechain-kit/src/utils/cssVariables.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/migration-guide-to-v2.mdc)
**/*.{ts,tsx}: In VeChain Kit Version 2, useuseThorinstead ofuseConnexfor contract interactions
For single contract read operations, use theuseCallClausehook with the pattern: import dependencies, define ABI and method as const, create query key function usinggetCallClauseQueryKeyWithArgs, and wrap with useCallClause including data transformation in the select option
For multiple parallel contract calls, use theexecuteMultipleClausesCallutility wrapped in auseQueryhook with the pattern: define query key function, useexecuteMultipleClausesCallin queryFn mapping items to clause objects, and transform results
For transaction building and sending, use theuseBuildTransactionhook with a clauseBuilder function that returns an array of clauses with optional comment fields describing the transaction action
Always provide an arguments array for contract calls, even when no parameters are required - use an empty array for parameter-less functions to enable TypeScript type checking
Always conditionally enable queries using theenabledproperty to prevent unnecessary contract calls, checking for all required parameters:enabled: !!requiredParam && !!otherRequiredParam
Use theselectoption in useCallClause or transform data in queryFn to handle data transformation, particularly for converting BigInt values to strings and normalizing contract return data
Maintain consistent query key patterns: usegetCallClauseQueryKeyWithArgsfor contract calls with arguments andgetCallClauseQueryKeyfor calls without arguments to ensure proper caching and invalidation
Use TypeScriptas constassertions for method names andas0x${string}`` assertions for Ethereum addresses to ensure type safety in contract interactions
Files:
packages/vechain-kit/src/components/common/BaseModal.tsxpackages/vechain-kit/src/providers/VeChainKitProvider.tsxpackages/vechain-kit/src/utils/cssVariables.tspackages/vechain-kit/src/theme/button.tspackages/vechain-kit/src/theme/modal.tspackages/vechain-kit/src/theme/tokens.ts
🧬 Code graph analysis (1)
packages/vechain-kit/src/components/common/BaseModal.tsx (1)
packages/vechain-kit/src/providers/VechainKitThemeProvider.tsx (1)
useVechainKitThemeConfig(190-198)
⏰ 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: Lint & Build
- GitHub Check: e2e-test
- GitHub Check: build-size-check
- GitHub Check: deploy
🔇 Additional comments (7)
packages/vechain-kit/src/theme/tokens.ts (2)
976-995: LGTM! Good backward compatibility pattern.The nullish coalescing with
config.modal?.rounded ?? config.modal?.borderRadiusproperly handles the deprecation while maintaining backward compatibility. The number-to-pixel conversion is also correctly implemented.
1177-1183: LGTM! Modal width conversion handles edge cases.The conversion logic correctly uses
convertChakraSizeToRemwith a fallback to default tokens, ensuring a valid width is always provided.packages/vechain-kit/src/theme/modal.ts (1)
15-15: LGTM! Proper Chakra UI rounded prop usage with fallback.The change from
borderRadiustoroundedaligns with Chakra UI's preferred prop naming, and the nullish coalescing fallback ensures backward compatibility whentokens.modal.roundedis undefined.packages/vechain-kit/src/providers/VeChainKitProvider.tsx (1)
612-621: LGTM! Proper propagation of modal width token.The modal width token is correctly passed to
applyPrivyCSSVariablesand included in the dependency array, ensuring Privy modals update when the theme changes.packages/vechain-kit/src/components/common/BaseModal.tsx (1)
57-70: LGTM! Modal width application logic is sound.The conditional logic correctly applies width constraints only when a custom
modalWidthis defined, preserving existing behavior otherwise. Setting bothwidthandmaxWensures consistent sizing.packages/vechain-kit/src/theme/button.ts (2)
19-21: LGTM! Consistent token-driven styling.The
roundedproperty with nullish coalescing fallback and optionalbackdropFilterfollow a consistent pattern. Chakra UI gracefully handlesundefinedvalues for both properties.
71-155: LGTM! All button variants consistently updated.The
vechainKitPrimary,vechainKitSecondary, andvechainKitTertiaryvariants all follow the same pattern forrounded(with fallback) andbackdropFilter(optional). This maintains consistency across the button system.
|
Size Change: -59.2 kB (-1.08%) Total Size: 5.4 MB
ℹ️ View Unchanged
|
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| tokens.colors.warning = defaultTokens.colors.warning; | ||
| } | ||
|
|
||
| // Handle modal border radius (support both borderRadius for backward compatibility and rounded) |
There was a problem hiding this comment.
New modal border-radius/backdropFilter handling added in convertThemeConfigToTokens increases module responsibilities and further enlarges an already-large tokens.ts, making the file harder to maintain.
Details
✨ AI Reasoning
1) The file defines ThemeTokens types, default token constants, numerous derive/merge/convert functions, and now additional modal/border-radius and backdropFilter handling spread across the file (new code blocks around lines 43, 924, and 972–990).
2) The added modal/borders/backdropFilter logic increases the file's responsibilities and surface area, mixing type definitions, defaults, parsing utilities, and complex config-to-token conversion in a single large module, which harms navigability and maintainability.
3) The violation is true because the PR introduced substantive new features and branching logic (modal radius handling and backdropFilter precedence) that further enlarge this already-large file and would benefit from splitting into smaller focused modules (e.g., button token extensions, modal/token conversion, and effects/backdrop handling).
🔧 How do I fix it?
Split large files into smaller, focused modules. Each file should have a single responsibility.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.