refactor(time-spinner): [date-picker] roll back some styles for time-spinner #2383
Conversation
WalkthroughThe changes in this pull request involve modifications to the styling of the time panel and time spinner components in the LESS files. Adjustments include the removal and addition of CSS properties, updates to CSS variables, and refinements to padding, margin, and height settings. The overall structure of the styles is maintained, but the visual layout and spacing of elements within the components are altered to enhance their appearance. Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/theme/src/time-panel/vars.less (1)
35-36: Consider enhancing the variable documentation.The new padding variable is well-structured and follows the established pattern. Consider adding a brief comment explaining the purpose of this bottom padding (e.g., if it's for creating space between content and footer).
// 内容区底部内间距 - --tv-TimePanel-content-padding-bottom: var(--tv-space-lg, 12px); + // 内容区底部内间距 - 用于在内容区和底部之间创建间隔 + --tv-TimePanel-content-padding-bottom: var(--tv-space-lg, 12px);packages/theme/src/time-panel/index.less (1)
Line range hint
108-109: Consider addressing or removing the TODO comment.The TODO comment about borderless scenarios should either be:
- Addressed by confirming if the borderless use case exists
- Removed if it's no longer relevant
Would you like me to help investigate if there are any borderless use cases in the codebase or create an issue to track this?
packages/theme/src/time-spinner/index.less (1)
46-46: Document the transform values.The transform values (-32px, -6px) appear to be magic numbers. Consider:
- Adding comments explaining why these specific values were chosen
- Converting these values to CSS variables for better maintainability
.@{time-spinner-prefix-cls}__list { - transform: translateY(-6px); + transform: translateY(var(--tv-TimeSpinner-list-offset, -6px)); margin: var(--tv-TimeSpinner-list-margin); }Also applies to: 56-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/theme/src/time-panel/index.less (1 hunks)
- packages/theme/src/time-panel/vars.less (1 hunks)
- packages/theme/src/time-spinner/index.less (7 hunks)
- packages/theme/src/time-spinner/vars.less (1 hunks)
🔇 Additional comments (8)
packages/theme/src/time-spinner/vars.less (2)
Line range hint
1-46: Verify the impact of removing--tv-TimeSpinner-arrow-padding.The removal of
--tv-TimeSpinner-arrow-paddingmight affect the arrow's appearance or positioning. Please ensure this change doesn't impact the component's visual consistency.Let's check how this variable was used:
#!/bin/bash # Search for any references to the removed arrow padding variable rg "TimeSpinner-arrow-padding" --type less -A 3 -B 3
32-33: Consider reducing the bottom margin in--tv-TimeSpinner-list-margin.The bottom margin of 30px seems disproportionately large compared to other spacing values in the component. This might create excessive vertical spacing in the time panel.
Let's check if this large margin is necessary:
packages/theme/src/time-panel/vars.less (1)
32-32: LGTM! Header margin changes align with the spacing system.The updated margin values properly utilize the design system's spacing variables (--tv-space-xl) while maintaining consistency with other components.
packages/theme/src/time-panel/index.less (2)
54-54: LGTM: Consistent use of CSS variable for padding.The addition of
padding-bottomusing the--tv-TimePanel-content-padding-bottomvariable maintains consistency with the project's design system approach.
Line range hint
31-41: Verify the impact of removed styles.According to the AI summary, some styles were removed:
- Height definition from header-input > input
- margin-top from header-title
Please verify that these removals don't adversely affect the component's layout.
Let's check for any fallback styles:
✅ Verification successful
Let me gather more specific information about the time-panel component to verify the impact of these style removals.
This will help us understand:
- The complete context of the time-panel styles
- Any CSS variable definitions that might provide fallback values
- Any mixins or common styles that might affect the component
Let me check one more thing to verify the variables and any potential default values:
This will help us understand:
- The default variable definitions in vars.less
- Any other locations where TimePanel variables might be defined or injected
The removal of these styles is safe and well-handled by CSS variables
After analyzing the complete time-panel styles and its variable definitions, I can confirm that:
- The removed
heightfrom header-input > input was not essential as there's no specific height requirement in the design system for this element.- The removed
margin-topfrom header-title is properly handled by the--tv-TimePanel-header-marginCSS variable, which provides a consistent spacing of16px 0 24px 0(defined in vars.less).The component's layout is maintained through:
- Proper margin definitions via
--tv-TimePanel-header-margin- Flex layout in the header-title (
display: flex; justify-content: space-around; align-items: center)- A comprehensive set of CSS variables that handle all spacing and sizing requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other height/margin definitions that might compensate for the removals # Search for height definitions in related files echo "Checking for input height definitions:" rg -l "height.*input" --type less packages/theme/src/ # Search for margin/padding definitions that might affect the header echo "Checking for header margin/padding definitions:" rg "(margin|padding).*header" --type less packages/theme/src/Length of output: 3217
Script:
#!/bin/bash # Check the complete time-panel styles and its dependencies echo "Checking time-panel styles:" cat packages/theme/src/time-panel/index.less echo -e "\nChecking for any CSS variable definitions that might provide default values:" rg "tv-TimePanel-" packages/theme/src/variables/ echo -e "\nChecking for any related mixins or common styles:" rg "time-panel|TimePanel" packages/theme/src/mixins/Length of output: 3613
Script:
#!/bin/bash # Check for TimePanel variables and the vars.less file echo "Checking TimePanel vars.less:" cat packages/theme/src/time-panel/vars.less echo -e "\nChecking for any other variable definitions:" rg -g "*.less" "inject-TimePanel-vars|--tv-TimePanel" packages/theme/src/Length of output: 6045
packages/theme/src/time-spinner/index.less (3)
34-34: Verify the increased scrollbar padding impact.The padding-bottom increase from 12px to 15px might affect the visual alignment with other elements. Please ensure this change doesn't create any visual inconsistencies, especially when the scrollbar is visible.
69-70: LGTM! Consistent arrow sizing and positioning.The changes establish a uniform height for arrows and consistent positioning, improving the overall visual harmony.
Also applies to: 88-88, 92-92
116-116: Verify the increased pseudo-element height impact.The height increase from 4px to 80px is substantial. Please ensure this change:
- Doesn't cause any layout issues in different viewport sizes
- Maintains proper spacing when the list is scrolling
- Works well with the existing padding and margin values
| // 时间项外间距 | ||
| --tv-TimeSpinner-item-margin: var(--tv-space-lg, 12px) var(--tv-space-md, 8px) 0 var(--tv-space-md, 8px); | ||
| --tv-TimeSpinner-item-margin: var(--tv-space-lg, 12px) 10px 0 10px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider maintaining theme adaptability.
The change from var(--tv-space-md, 8px) to static 10px for left/right margins reduces the component's ability to adapt to different themes and spacing scales. Consider keeping the variable-based approach:
--- --tv-TimeSpinner-item-margin: var(--tv-space-lg, 12px) 10px 0 10px;
+++ --tv-TimeSpinner-item-margin: var(--tv-space-lg, 12px) var(--tv-space-md, 8px) 0 var(--tv-space-md, 8px);📝 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.
| --tv-TimeSpinner-item-margin: var(--tv-space-lg, 12px) 10px 0 10px; | |
| --tv-TimeSpinner-item-margin: var(--tv-space-lg, 12px) var(--tv-space-md, 8px) 0 var(--tv-space-md, 8px); |
| @@ -122,6 +125,7 @@ | |||
| line-height: var(--tv-TimeSpinner-line-height); | |||
| color: var(--tv-TimeSpinner-text-color); | |||
| border-radius: var(--tv-TimeSpinner-item-border-radius); | |||
| width: 52px; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a CSS variable for item width.
For better maintainability and consistency, consider moving the explicit width to a CSS variable.
- width: 52px;
+ width: var(--tv-TimeSpinner-item-width, 52px);📝 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.
| width: 52px; | |
| width: var(--tv-TimeSpinner-item-width, 52px); |
MomoPoppy
changed the title
|
|
|
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
Chores