refactor(calendar-view): [calendar-view] refactor theme vars #2297
Conversation
WalkthroughThe changes in this pull request involve modifications to multiple files related to the calendar view component. Key updates include adjustments to test cases for element selection, significant styling updates in CSS files to enhance layout and structure, and the introduction of new CSS variables. Additionally, properties were removed from an old theme object, and the structure of a Vue component was simplified by altering the header layout. Overall, the changes reflect a comprehensive update to improve the calendar view's functionality and appearance. Changes
Suggested labels
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: 4
🧹 Outside diff range and nitpick comments (5)
packages/theme/src/calendar-view/vars.less (3)
18-27: Consider more descriptive names for some variables.While the overall structure is good, some variable names could be more descriptive. For example:
--tv-CalendarView-radio-icon-fillcould be--tv-CalendarView-radio-icon-color--tv-CalendarView-header-main-text-sizecould be--tv-CalendarView-header-title-font-sizeThese changes would make the purpose of each variable more immediately clear.
28-47: Consider grouping related variables and adding comments.The variables in this section cover various aspects of the calendar's main view. To improve readability and maintainability, consider:
- Grouping related variables together (e.g., all date-related variables, all selection-related variables).
- Adding comments to separate these groups.
For example:
// Date styles --tv-CalendarView-date-margin-left: var(--tv-space-xl); --tv-CalendarView-date-text-color: var(--tv-color-text); --tv-CalendarView-date-weaken-text-color: var(--tv-color-text-weaken); // Selection styles --tv-CalendarView-date-selected-text-color: var(--tv-color-text-active); --tv-CalendarView-li-selected-bg: var(--tv-color-bg-active-emphasize-light); --tv-CalendarView-selected-hover-color: var(--tv-color-bg-active-control);This organization will make it easier for developers to find and modify related styles.
48-69: Well-structured event and tooltip variables.The variables for events and tooltips are well-organized and provide comprehensive styling options. The naming is clear and descriptive, which will aid in maintenance and usage.
One minor suggestion: Consider adding a comment to separate the week mode variables (starting from line 65) for better visual organization.
// Week mode styles --tv-CalendarView-week-date-font-szie: var(--tv-font-size-lg); --tv-CalendarView-week-date-text-color: var(--tv-color-text); // ... other week mode variablesThis will help developers quickly identify and modify week-specific styles.
packages/theme/src/calendar-view/index.less (2)
51-51: Remove the extra semicolon at the end of line 51The double semicolon is unnecessary and can be removed for cleaner syntax.
Apply this diff to fix:
- height: var(--tv-CalendarView-radio-font-size);; + height: var(--tv-CalendarView-radio-font-size);
468-468: Remove the extra semicolon at the end of line 468An extra semicolon is present and can be removed for cleaner code.
Apply this diff to fix:
- padding: var(--tv-CalendarView-events-list-padding);; + padding: var(--tv-CalendarView-events-list-padding);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- examples/sites/demos/pc/app/calendar-view/calendar-disabled-day.spec.ts (1 hunks)
- packages/theme/src/calendar-view/index.less (17 hunks)
- packages/theme/src/calendar-view/old-theme.js (0 hunks)
- packages/theme/src/calendar-view/vars.less (1 hunks)
- packages/vue/src/calendar-view/src/pc.vue (1 hunks)
💤 Files with no reviewable changes (1)
- packages/theme/src/calendar-view/old-theme.js
🧰 Additional context used
🔇 Additional comments (5)
packages/theme/src/calendar-view/vars.less (2)
13-17: Improved naming convention and structure.The changes in this section show a significant improvement in the naming convention and structure of the CSS variables. The new function name
.inject-CalendarView-vars()is more descriptive and follows a consistent pattern. The use of--tv-CalendarView-prefix for all variables enhances readability and maintainability.
13-102: Overall, excellent refactoring of calendar view variables.This refactoring of the calendar view variables represents a significant improvement in the organization, naming convention, and comprehensiveness of the styling options. The new structure with the
.inject-CalendarView-vars()function and the consistent--tv-CalendarView-prefix for all variables enhances readability and maintainability.Key improvements:
- Consistent naming convention
- Comprehensive coverage of styling options
- Use of semantic variable names
- Leveraging existing theme variables for consistency
The suggested minor improvements (grouping, comments, and fixing the typo) will further enhance the usability of this file. Great work on this refactoring!
packages/vue/src/calendar-view/src/pc.vue (1)
4-5: LGTM! Consider verifying button styling.The restructuring of the header section simplifies the DOM, which is a positive change for maintainability. The button's functionality remains intact.
Please verify that the removal of the
tiny-calendar-view__tool-firstclass doesn't negatively impact the button's styling or layout. If necessary, update the CSS to maintain the desired appearance.✅ Verification successful
Verified: Removal of
tiny-calendar-view__tool-firstclass does not affect button styling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any CSS rules targeting the removed class and for any new CSS rules for the button's new parent div. # Search for CSS rules using the old class echo "CSS rules using the old class:" rg --type css 'tiny-calendar-view__tool-first' # Search for new CSS rules that might have been added for the new structure echo "New CSS rules for the button's parent div:" rg --type css 'tiny-calendar-view__header\s*>\s*div'Length of output: 270
Script:
#!/bin/bash # Description: Verify that the removed class 'tiny-calendar-view__tool-first' is no longer used and check for any inline or scoped styles affecting the button. # Search for the removed class in all CSS and Vue files echo "Searching for 'tiny-calendar-view__tool-first' in CSS and Vue files:" rg 'tiny-calendar-view__tool-first' --type css --type vue # Search for any inline styles on the <tiny-button> component echo "Searching for inline styles on <tiny-button> components in Vue files:" rg '<tiny-button[^>]*style=' --type vue # Search for scoped styles in Vue files that might affect the button echo "Searching for scoped styles related to the button in Vue files:" rg '<style scoped>' --type vueLength of output: 655
Script:
#!/bin/bash # Description: Verify that the removed class 'tiny-calendar-view__tool-first' is no longer used and check for any inline or scoped styles affecting the button. # Search for the removed class in all CSS and Vue files echo "Searching for 'tiny-calendar-view__tool-first' in CSS and Vue files:" rg 'tiny-calendar-view__tool-first' --glob "*.css" --glob "*.vue" # Search for any inline styles on the <tiny-button> component echo "Searching for inline styles on <tiny-button> components in Vue files:" rg '<tiny-button[^>]*style=' --glob "*.vue" # Search for scoped styles in Vue files that might affect the button echo "Searching for scoped styles related to the button in Vue files:" rg '<style scoped>' --glob "*.vue"Length of output: 97534
packages/theme/src/calendar-view/index.less (2)
233-233: Verify the variable name for potential unintended double hyphens at line 233The CSS variable
--tv-CalendarView-event-title--text-colorcontains a double hyphen beforetext-color. Ensure this is intentional and matches the variable defined invars.less.If unintended, apply this diff to correct it:
- color: var(--tv-CalendarView-event-title--text-color); + color: var(--tv-CalendarView-event-title-text-color);If this variable is widely used, please verify its consistency across all relevant files.
356-356:⚠️ Potential issueCorrect the typo in variable name from 'font-szie' to 'font-size' in line 356
There's a typo in the CSS variable name. Replace
font-sziewithfont-sizeto ensure the variable is correctly referenced.Apply this diff to fix the typo:
- font-size: var(--tv-CalendarView-week-date-font-szie); + font-size: var(--tv-CalendarView-week-date-font-size);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
Refactor
Chores