feat(date-picker): [date-picker] Adapting to the old-theme #2575
Conversation
WalkthroughThe pull request introduces modifications to several LESS stylesheet files, focusing on theme variables and CSS selectors. Key changes include the update of a color variable for active background states in the Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/month-table/index.less (1)
144-147: Document the layout fix for future maintenanceThe addition of
box-sizing: border-boxappears to be addressing a specific layout issue with the old theme. Consider adding a comment explaining why this property is necessary specifically for cells not in range.&:not(.in-range) { .cell { + // Fix cell sizing inconsistency in old theme by including padding and border in width calculation box-sizing: border-box; width: auto; border-radius: var(--tv-MonthTable-cell-border-radius); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/theme/src/base/old-theme.less(1 hunks)packages/theme/src/date-table/index.less(1 hunks)packages/theme/src/month-table/index.less(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/base/old-theme.less
🔇 Additional comments (1)
packages/theme/src/month-table/index.less (1)
144-144: Verify box-sizing consistency across all cell states
Adding box-sizing: border-box only to cells not in range could create inconsistent sizing behavior between different cell states. Consider applying this consistently across all cell states to maintain uniform sizing calculations.
| &__row td:first-of-type div { | ||
| border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| } | ||
| &__row .in-range:last-of-type div { | ||
| &__row td:last-of-type div { | ||
| border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| } | ||
| &__row td:nth-child(1 of .available.in-range) div { | ||
| &__row td.pre-month + td.available.in-range div { | ||
| border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| } | ||
| &__row td:nth-last-child(1 of .available.in-range) div { | ||
| &__row td.available.in-range:has(+ td.next-month) div { | ||
| border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled); |
There was a problem hiding this comment.
💡 Codebase verification
The border radius variable usage is incorrect
The code is using --tv-DateTable-td-border-radius-disabled for non-disabled states, which is inconsistent with the codebase patterns. The search results show that:
- The codebase uses
--tv-DateTable-td-border-radiusfor normal states --tv-DateTable-td-border-radius-disabledis specifically used for disabled states
The code should be modified to use the correct variables:
&__row td:first-of-type div {
- border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-left-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
}
&__row td:last-of-type div {
- border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-right-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
}
&__row td.pre-month + td.available.in-range div {
- border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-left-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
}
&__row td.available.in-range:has(+ td.next-month) div {
- border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-right-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
}🔗 Analysis chain
Review the border radius variable usage and browser compatibility
There are several concerns with the new selectors:
-
The selectors are using
--tv-DateTable-td-border-radius-disabledvariable for non-disabled states, which seems incorrect. Consider using a more appropriate variable like--tv-DateTable-td-border-radiusfor normal states. -
The
:has()selector (line 275) has limited browser support. Consider providing a fallback for better browser compatibility.
Consider applying this refactor for better maintainability and consistency:
- &__row td:first-of-type div {
- border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- }
-
- &__row td:last-of-type div {
- border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- }
-
- &__row td.pre-month + td.available.in-range div {
- border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- }
-
- &__row td.available.in-range:has(+ td.next-month) div {
- border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- }
+ // Border radius for normal states
+ &__row {
+ td:first-of-type div {
+ border-top-left-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
+ }
+
+ td:last-of-type div {
+ border-top-right-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
+ }
+
+ // Range selection borders
+ td.pre-month + td.available.in-range div {
+ border-top-left-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
+ }
+
+ // Fallback for browsers that don't support :has
+ td.available.in-range.next-month-boundary div {
+ border-top-right-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
+ }
+
+ // Modern browsers
+ @supports selector(:has(*)) {
+ td.available.in-range:has(+ td.next-month) div {
+ border-top-right-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
+ }
+ }
+ }This refactor:
- Uses appropriate border radius variables
- Provides better organization of selectors
- Adds browser compatibility support
- Maintains the same functionality while being more maintainable
Let's verify the browser support for the :has selector:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check browser support for :has selector
curl -s "https://caniuse.com/api/features?feature=css-has" | jq '.data."css-has".support'
Length of output: 91
Script:
#!/bin/bash
# Let's check if there are any existing CSS variables defined for border radius
rg "tv-DateTable-td-border-radius" -A 2 -B 2
# Also check for any other border radius variables that might be more appropriate
rg "border-radius.*var\(--" -A 2 -B 2
Length of output: 104115
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/theme/src/month-table/index.less (1)
Line range hint
1-158: Consider documenting theme-specific layout behaviorsSince this is part of adapting the date-picker to the old theme, consider:
- Documenting these layout changes in the theme documentation
- Creating theme-specific variables for layout properties to make future theme switches easier
- Ensuring consistent box-model behavior across all date-picker related components (date-table, month-table, year-table)
Consider extracting common layout properties into theme variables:
&:not(.in-range) { .cell { - box-sizing: border-box; - width: auto; + box-sizing: var(--tv-MonthTable-cell-box-sizing, border-box); + width: var(--tv-MonthTable-cell-width, auto); border-radius: var(--tv-MonthTable-cell-border-radius); } }packages/theme/src/date-table/index.less (1)
263-278: Consider extracting border radius logic into theme variablesTo improve maintainability and theme consistency, consider extracting the border radius logic into dedicated theme variables. This would:
- Make it easier to maintain consistent styling across different states
- Allow for better theme customization
- Reduce code duplication
Consider adding these variables to
vars.less:
--tv-DateTable-range-border-radius--tv-DateTable-edge-border-radiusThen use them consistently across all border radius definitions in this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/theme/src/base/old-theme.less(1 hunks)packages/theme/src/date-table/index.less(1 hunks)packages/theme/src/month-table/index.less(1 hunks)
🔇 Additional comments (3)
packages/theme/src/month-table/index.less (1)
144-147: Verify layout consistency with the old theme
The addition of box-sizing: border-box is a good practice for predictable layouts. However, changing the width from 100% to auto might affect cell alignment and spacing.
Please ensure that:
- The cell alignment remains consistent across different states (in-range vs not-in-range)
- The layout matches the old theme's behavior exactly
Let's verify the old theme's implementation:
packages/theme/src/base/old-theme.less (2)
348-348: LGTM! Color variable change aligns with documented usage.
The change from var(--tv-base-color-brand) to var(--tv-base-color-brand-2) for --tv-color-bg-active-emphasize is appropriate, as --tv-base-color-brand-2 is specifically documented for date selection hover background color in the comments.
348-348: Verify the usage of the color variable in date-picker styles.
Let's confirm that this color variable is being used appropriately in the date-picker component's styles.
✅ Verification successful
Color variable is correctly used in date-picker related components
The variable --tv-color-bg-active-emphasize is properly used in date-picker related components:
- In
date-tablecomponent for range selection and week selection backgrounds - In
time-spinnercomponent for active item backgrounds - The color change from
#5e7ce0tovar(--tv-base-color-brand-2)maintains theme consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of the --tv-color-bg-active-emphasize variable in date-picker related files
# Search for the variable usage in LESS/CSS files
rg --type=less --type=css "tv-color-bg-active-emphasize"
# Search specifically in date-picker related files
fd -e less -e css "date" | xargs rg "tv-color-bg-active-emphasize"
Length of output: 19860
| &__row td:first-of-type div { | ||
| border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| } | ||
| &__row .in-range:last-of-type div { | ||
| &__row td:last-of-type div { | ||
| border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review border radius styling for first/last cells
The current implementation has several concerns:
- Using
--tv-DateTable-td-border-radius-disabledfor all cells might not be appropriate as it's meant for disabled states - The broad selectors
td:first-of-typeandtd:last-of-typewill apply border radius to all cells, which might not be desired for middle cells in a range selection
Consider using a more specific approach:
- &__row td:first-of-type div {
+ &__row td:first-of-type:not(.in-range) div {
border-top-left-radius: var(--tv-DateTable-td-border-radius);
border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
}
- &__row td:last-of-type div {
+ &__row td:last-of-type:not(.in-range) div {
border-top-right-radius: var(--tv-DateTable-td-border-radius);
border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &__row td:first-of-type div { | |
| border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled); | |
| border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled); | |
| } | |
| &__row .in-range:last-of-type div { | |
| &__row td:last-of-type div { | |
| border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled); | |
| border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled); | |
| } | |
| &__row td:first-of-type:not(.in-range) div { | |
| border-top-left-radius: var(--tv-DateTable-td-border-radius); | |
| border-bottom-left-radius: var(--tv-DateTable-td-border-radius); | |
| } | |
| &__row td:last-of-type:not(.in-range) div { | |
| border-top-right-radius: var(--tv-DateTable-td-border-radius); | |
| border-bottom-right-radius: var(--tv-DateTable-td-border-radius); | |
| } |
| &__row td.pre-month + td.available.in-range div { | ||
| border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| } | ||
| &__row td:nth-last-child(1 of .available.in-range) div { | ||
| &__row td.available.in-range:has(+ td.next-month) div { | ||
| border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Revise range border styling implementation
The verification reveals that:
- The
:has()selector is widely used across the codebase, and the project's browserslist configuration ("> 1%, last 2 versions") suggests modern browser support is acceptable - The
--tv-DateTable-td-border-radius-disabledvariable is consistently used for disabled states, while--tv-DateTable-td-border-radiusis used for normal states, confirming that using the disabled variant for range cells is incorrect - The suggested alternative using
:last-childmight miss edge cases, as the current:has()implementation is more precise for handling next-month transitions
Recommended changes:
&__row td.pre-month + td.available.in-range div {
- border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-left-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
}
&__row td.available.in-range:has(+ td.next-month) div {
- border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-right-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
}🔗 Analysis chain
Review range border styling implementation
The current implementation has several concerns:
- The
:has()selector has limited browser support - Using
--tv-DateTable-td-border-radius-disabledfor range cells seems incorrect - The current approach might not handle all edge cases correctly
Consider the following improvements:
&__row td.pre-month + td.available.in-range div {
- border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-left-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
}
- &__row td.available.in-range:has(+ td.next-month) div {
+ &__row td.available.in-range:last-child div,
+ &__row td.available.in-range + td.next-month div {
- border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
- border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+ border-top-right-radius: var(--tv-DateTable-td-border-radius);
+ border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
}Let's verify browser compatibility requirements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any browser compatibility requirements in the project
rg -l "browserslist|@supports|\.browserlist" --type yaml --type json
Length of output: 116
Script:
#!/bin/bash
# Check the browser compatibility configuration in package.json files
cat package.json packages/theme-saas/package.json | jq -r '.browserslist // empty'
# Check if :has() selector is already used elsewhere in the codebase
rg -l ":has\(" --type css --type less
# Check for existing border radius variables in theme files
rg "tv-DateTable-td-border-radius" --type css --type less
Length of output: 3768
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
box-sizing: border-box;to month table cells for better layout handling.Bug Fixes