refactor(calendar): [calendar] refactor theme vars #2323
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the LESS stylesheets for a calendar component. Key updates include renaming a mixin, changing CSS variable prefixes, and standardizing dimensions and colors. The updates reflect a new theming approach with a focus on fixed values rather than variables for certain properties, enhancing consistency in the calendar's visual presentation. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
packages/theme/src/calendar/index.less (4)
114-114: Approve theme variables for colors, consider variable for icon size.The changes to icon styling improve theme consistency:
- The use of
--tv-Calendar-text-color-primaryand--tv-Calendar-hover-text-colorfor fill colors is good for maintainability and theming.- Consider using a theme variable for the icon font size instead of a fixed value (14px) to allow for more flexibility:
- font-size: 14px; + font-size: var(--tv-Calendar-icon-font-size);Also applies to: 120-120, 123-123
177-177: Approve use of theme variable for calendar header font size.The calendar header font size now uses the theme variable
--tv-Calendar-content-heard-font-size. This change improves maintainability and ensures consistency across the component.There's a typo in the variable name: "heard" should be "header". Consider fixing this in the variable definition.
195-195: Approve theme variables, suggest variable for border color.The calendar day styling has been updated:
- Approve the use of theme variables for background and text colors in different states (selected, hover). This improves theming capabilities.
- The border color has been changed to a fixed color (#999999). Consider using a theme variable for better flexibility:
- border-top: 3px solid #999999; + border-top: 3px solid var(--tv-Calendar-day-border-color);Also applies to: 201-201, 204-204, 209-209
282-282: Approve use of theme variable, consider separate variable for tip header.The calendar tip header font size now uses the theme variable
--tv-Calendar-content-heard-font-size. While this improves maintainability, consider the following:
- There's a typo in the variable name: "heard" should be "header".
- The tip header and the main content header might require different sizes. Consider using a separate variable for the tip header:
- font-size: var(--tv-Calendar-content-heard-font-size); + font-size: var(--tv-Calendar-tip-header-font-size);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/calendar/index.less (12 hunks)
- packages/theme/src/calendar/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (11)
packages/theme/src/calendar/vars.less (1)
13-57: Summary of changes and potential impactThis refactoring of the calendar theme variables represents a significant improvement in the customization capabilities of the calendar component. The changes include:
- A new naming convention with the
--tv-prefix.- More comprehensive coverage of different states and elements within the calendar.
- Better organization and documentation of variables.
These changes will likely improve the maintainability and flexibility of the calendar component's styling. However, it's important to note that this is a breaking change for any existing code that relies on the old variable names.
To ensure that these changes are consistently applied across the project, please run the following verification script:
This script will help identify any remaining instances of old variable usage and confirm the adoption of the new variables across the project.
packages/theme/src/calendar/index.less (10)
25-25: Consistent application of new variable naming convention.The background color variable has been updated to use the new
--tv-prefix and PascalCase for the component name. This change is consistent with the new naming convention observed throughout the file.
40-41: Improved theming flexibility for selected state.The selected state styling has been updated to use the new
--tv-prefixed variables. This change not only aligns with the new naming convention but also provides more granular control over the theming of the selected state.Also applies to: 43-43, 45-45
71-71: Consider flexibility vs. consistency in input field dimensions.The input field styling has been updated with fixed dimensions and new theme variables. While this improves consistency, it might reduce flexibility for different use cases.
- The fixed width (104px) and height (32px) ensure consistent sizing but may limit adaptability to different layouts. Consider if these should remain as variables for more flexibility.
- The use of new
--tv-prefixed variables for colors, border-radius, and font-size is good for maintainability and theming.Could you confirm if the fixed dimensions are intentional and if they meet all use case requirements?
Also applies to: 77-83
135-138: Consider flexibility in tab dimensions, approve use of theme variables.The tabs styling has been updated with fixed dimensions and theme variables:
- Fixed dimensions (width: 52px, height: 32px) ensure consistency but might limit flexibility. Consider if these should be theme variables for adaptability to different designs.
- The use of
--tv-Calendar-input-font-sizeand--tv-Calendar-text-color-infois good for maintainability and theming.Could you confirm if the fixed tab dimensions are intentional and meet all design requirements?
Also applies to: 140-140, 142-142
160-161: Approve use of theme variables for active tab styling.The active tab styling has been updated to use the new theme variables
--tv-Calendar-text-color-primary. This change is consistent with the new naming convention and improves maintainability and theming capabilities.
213-213: Approve enhanced theming capabilities for disabled states.The disabled day styling has been significantly improved:
- New theme variables are used for different states (info, success, warning, error) in the disabled context.
- This change provides great flexibility for theming and is consistent with the new naming convention.
These updates allow for more granular control over the appearance of disabled days, which can greatly enhance the user experience and accessibility of the calendar.
Also applies to: 216-216, 220-220, 224-224, 228-228
234-234: Approve use of theme variable for current month and today styling.The border color for the current month and today now uses the theme variable
--tv-Calendar-text-color-primary. This change is consistent with the new naming convention and improves theming capabilities, allowing for easier customization of these important calendar states.
258-258: Approve enhanced theming capabilities for event indicators.The event indicator styling has been improved:
- New theme variables are used for different event types (info, success, warning, error).
- This change provides great flexibility for theming and is consistent with the new naming convention.
These updates allow for more granular control over the appearance of event indicators, which can enhance the visual distinction between different event types in the calendar.
Also applies to: 262-262, 266-266, 270-270
300-300: Consider flexibility in dimensions, approve use of theme variables for colors.The calendar selector and list item styling has been updated:
- Fixed dimensions are used for the selector width (104px) and list item height (32px). Consider if these should be theme variables for more flexibility across different designs.
- Approve the use of theme variables for colors in different states (normal, hover, selected). This improves theming capabilities and is consistent with the new naming convention.
Could you confirm if the fixed dimensions for the selector and list items are intentional and meet all design requirements?
Also applies to: 307-308, 314-314, 318-318, 322-323
Line range hint
1-325: Overall assessment: Improved theming with some considerationsThe changes in this file significantly improve the theming capabilities of the calendar component:
- The new
--tv-prefixed variables provide more granular control over various aspects of the calendar's appearance.- The consistent use of theme variables for colors enhances maintainability and customization options.
However, there are a few areas that could be further improved:
- Consider using theme variables for dimensions (e.g., input width, tab width) to maintain flexibility for different designs.
- Replace remaining hardcoded colors (e.g., #191919, #999999) with theme variables for complete theming control.
- Fix the typo in the mixin name (
injiect→inject) and the content header variable name (heard→header).- Consider separate variables for elements that might need different sizes (e.g., main content header vs. tip header).
Overall, these changes represent a significant improvement in the component's theming capabilities, with minor adjustments needed for optimal flexibility and consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/calendar/vars.less (1)
15-59: Consider using lowercase "calendar" in variable namesFor better consistency with CSS naming conventions, consider changing "Calendar" to "calendar" in all variable names. This would improve readability and align with common CSS practices.
For example:
---tv-Calendar-bg-color +--tv-calendar-bg-colorThis change should be applied to all variables in the mixin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/calendar/index.less (12 hunks)
- packages/theme/src/calendar/vars.less (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/calendar/index.less
🧰 Additional context used
🔇 Additional comments (2)
packages/theme/src/calendar/vars.less (2)
13-13: LGTM: Mixin name corrected and structure improvedThe mixin name has been corrected to
.inject-Calendar-vars(), addressing the typo mentioned in a previous review. The use of a mixin for injecting variables is a good practice for maintaining modular and reusable CSS.
51-59: 🛠️ Refactor suggestionConsider differentiating disabled states
Currently, all disabled states (info, success, warning, error) use the same background color (
var(--tv-color-bg-secondary)). This might not provide enough visual distinction between these states when disabled.Consider using slightly different shades or opacities for each disabled state to maintain visual hierarchy:
---tv-Calendar-disabled-info-bg-color: var(--tv-color-bg-secondary); ---tv-Calendar-disabled-success-bg-color: var(--tv-color-bg-secondary); ---tv-Calendar-disabled-warning-bg-color: var(--tv-color-bg-secondary); ---tv-Calendar-disabled-error-bg-color: var(--tv-color-bg-secondary); +--tv-Calendar-disabled-info-bg-color: var(--tv-color-info-bg-disabled); +--tv-Calendar-disabled-success-bg-color: var(--tv-color-success-bg-disabled); +--tv-Calendar-disabled-warning-bg-color: var(--tv-color-warning-bg-disabled); +--tv-Calendar-disabled-error-bg-color: var(--tv-color-error-bg-disabled);Ensure that these new variables are defined in your global theme with appropriate values that provide subtle differentiation while maintaining the disabled appearance.
Likely invalid or redundant comment.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation