feat(date-picker): [date-picker]Optimize case display and fix style issues #2400
Conversation
WalkthroughThe pull request consists of various changes primarily focused on the styling and layout of date picker components across multiple Vue files. Key modifications include adjusting the width of the 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 (
|
Youyou-smiles
changed the title
![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 (1)
examples/sites/demos/pc/app/date-picker/now.vue (1)
4-4: Consider internationalizing the UI text.The component uses Chinese text which might limit its accessibility for non-Chinese speakers. Consider using i18n for better international support.
Example implementation:
- <div class="title">插槽式：</div> + <div class="title">{{ $t('datePicker.slotBased') }}</div> - <div class="title">函数式：</div> + <div class="title">{{ $t('datePicker.functionBased') }}</div>Also applies to: 13-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- examples/sites/demos/pc/app/date-picker/align-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/align.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/date-range-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/date-range.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/events-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/events.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/now-composition-api.vue (3 hunks)
- examples/sites/demos/pc/app/date-picker/now.vue (3 hunks)
- examples/sites/demos/pc/app/date-picker/timezone-composition-api.vue (1 hunks)
- packages/renderless/src/date-panel/vue.ts (1 hunks)
- packages/theme/src/date-table/index.less (1 hunks)
- packages/theme/src/month-table/index.less (1 hunks)
- packages/theme/src/month-table/vars.less (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- examples/sites/demos/pc/app/date-picker/align-composition-api.vue
- examples/sites/demos/pc/app/date-picker/date-range-composition-api.vue
- examples/sites/demos/pc/app/date-picker/date-range.vue
- examples/sites/demos/pc/app/date-picker/timezone-composition-api.vue
🔇 Additional comments (13)
examples/sites/demos/pc/app/date-picker/now-composition-api.vue (2)
3-3: Verify if width reduction impacts alarm visibility.The change from inline
width: 400pxto a CSS class withwidth: 280pxmight affect the user experience when displaying a large number of alarms. Could you please clarify:
- How this width reduction aligns with the PR objective of modifying console for large numbers of alarms?
- Whether any usability testing has been performed to ensure alarm information remains readable at this narrower width?
Also applies to: 12-12
Line range hint
1-57: Request clarification on missing console modifications.The PR objective mentions "Modify console for a large number of alarms", but this file only contains styling changes. Could you please:
- Clarify where the console modifications for handling large numbers of alarms are implemented?
- Confirm if additional files need to be included in this PR to achieve the stated objective?
examples/sites/demos/pc/app/date-picker/now.vue (2)
3-3: LGTM! Good improvement in styling approach.The change from inline styles to CSS classes improves maintainability and follows Vue.js best practices.
Also applies to: 12-12
Line range hint
1-65: Verify alignment with PR objectives.The changes in this file focus on styling improvements, but the PR description mentions modifying the console for handling large numbers of alarms. Could you clarify how these changes relate to the alarm handling functionality?
Let's check for any alarm-related code in the codebase:
examples/sites/demos/pc/app/date-picker/events-composition-api.vue (2)
18-21: LGTM! Layout adjustment improves visual consistency.The span reduction from 12 to 6 for the daterange picker aligns better with the layout of other date pickers in the demo, creating a more balanced and consistent appearance.
Line range hint
1-65: Verify alignment between PR description and implementation.The PR title mentions modifications for "a large number of alarms", but the changes in this file appear to be purely layout-related. Could you please clarify how these layout changes relate to the alarm handling functionality mentioned in the PR title?
Let's search for alarm-related code in the codebase:
examples/sites/demos/pc/app/date-picker/events.vue (2)
18-21: PR objective seems inconsistent with the changes.The PR title mentions "Modify console for a large number of alarms", but the changes only adjust the layout width. There's no visible modification to handle alarms or console-related functionality. Please clarify:
- Where are the console modifications for handling alarms?
- How does this layout change relate to the alarm handling functionality?
#!/bin/bash # Description: Search for alarm-related code in date-picker files # Expected: Find code handling alarms if this PR is correctly implementing its objective # Search for alarm-related code rg -i "alarm" examples/sites/demos/pc/app/date-picker/
18-21: Verify consistent layout changes across related files.The span reduction from 12 to 6 appears to be part of a larger effort to make the layout more compact. Let's verify this change is consistent with other files.
✅ Verification successful
Layout changes are consistent within date picker demos
The span values are consistent across both date picker event demo files (
events.vueandevents-composition-api.vue). Both files follow the same pattern:
- Three columns with
:span="4"- One column with
:span="6"for the onPick sectionThis indicates the layout change is intentionally designed and consistently implemented within the date picker component demos.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent span values in related date-picker demo files # Expected: All onPick date pickers should use span="6" # Search for tiny-col spans in date picker demo files rg -U "tiny-col.*:span.*onPick" examples/sites/demos/pc/app/date-picker/Length of output: 177
Script:
#!/bin/bash # Let's try a broader search to understand the layout patterns # 1. Find all tiny-col spans in the date-picker directory rg -U "tiny-col.*:span" examples/sites/demos/pc/app/date-picker/ # 2. Also check if there are other similar demo files fd "events.vue" examples/sites/demos/pc/app/Length of output: 2381
packages/theme/src/month-table/vars.less (1)
45-46: Consider potential variable duplication.The new variable
--tv-MonthTable-cell-color-disabledappears to serve a similar purpose as the existing--tv-MonthTable-text-color-disabledvariable. While the implementation is correct, having two variables for seemingly the same purpose could lead to confusion.Let's check how these variables are used:
Could you clarify the distinct use cases for these variables? If they serve the same purpose, consider consolidating them to maintain consistency.
packages/theme/src/month-table/index.less (1)
67-71: LGTM: Improved disabled state stylingThe changes enhance the visual feedback for disabled cells by:
- Using a dedicated color variable for disabled text
- Maintaining consistent background color on hover
packages/renderless/src/date-panel/vue.ts (1)
138-138: Verify the impact of removing the default button size.Changing the fallback value from 'default' to an empty string could be a breaking change that affects button styling consistency across the application. Additionally, this change seems unrelated to the PR's objective of modifying console for handling large number of alarms.
Let's verify the button size usage across the codebase:
Consider the following recommendations:
- Document this change in the changelog if it's intentional
- Add a comment explaining why the default value is being removed
- Consider keeping the default value for backward compatibility
✅ Verification successful
Removing default button size is safe in this context
The change from 'default' to empty string as fallback is safe because:
- The
buttonSizeproperty in date-panel is already controlled bydesignConfigwith a defined value of 'mini' inpackages/design/saas/src/date-panel/index.ts- No components were found relying on the 'default' fallback value
- The button size system supports multiple sizes (mini, small, medium, large) as seen in the design tokens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential impacts of buttonSize change # Test 1: Find components that might be affected by this change rg -l "buttonSize|button-size" --type vue # Test 2: Find direct references to 'default' button size rg "buttonSize.*default|button-size.*default" --type vue # Test 3: Check if other components rely on the default button size ast-grep --pattern 'buttonSize = $value'Length of output: 201
Script:
#!/bin/bash # Let's try a different approach without file type restrictions # Test 1: Find files containing buttonSize or button-size rg -l "buttonSize|button-size" # Test 2: Find direct references to buttonSize with context rg -A 3 "buttonSize.*default|button-size.*default" # Test 3: Check for any imports or usage of the date-panel component rg -l "date-panel" # Test 4: Look for any design configuration files that might define button sizes fd -e json -e js -e ts | xargs rg "buttonSize|button-size"Length of output: 4644
packages/theme/src/date-table/index.less (2)
244-246: Clarify the relationship with alarm handling.While this styling change enhances the visual feedback for selected date ranges, it's unclear how it relates to the PR's objective of "modifying console for a large number of alarms". Please clarify the connection or update the PR description to accurately reflect the changes.
#!/bin/bash # Search for alarm-related code or comments rg -i "alarm" --type vue --type js --type less
244-246: Verify the CSS variable definition.The new style rule uses
var(--tv-DateTable-td-bg-color-range-selected). Please ensure this variable is properly defined in the theme variables file.✅ Verification successful
CSS variable is properly defined and used correctly ✓
The variable
--tv-DateTable-td-bg-color-range-selectedis properly defined inpackages/theme/src/date-table/vars.lesswith a fallback color value:--tv-DateTable-td-bg-color-range-selected: var(--tv-color-bg-active-emphasize, #deecff);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the CSS variable definition rg --type less "tv-DateTable-td-bg-color-range-selected" -A 2Length of output: 168
Script:
#!/bin/bash # Search for CSS variable definitions in theme-related files fd -e less -e css | xargs rg --type less "\-\-tv\-DateTable\-td\-bg\-color\-range\-selected" # Also search in common theme or variable files fd "variables|common|theme" -e less -e css | xargs rg --type less "\-\-tv\-DateTable\-td\-bg\-color\-range\-selected" # Search for any similar variable patterns to understand naming convention fd -e less -e css | xargs rg --type less "\-\-tv\-DateTable\-td\-bg\-color"Length of output: 780
| .demo-date-picker { | ||
| width: 280px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding responsive width handling.
While standardizing the width is good, consider making the date picker more responsive to different screen sizes and device types. This would be particularly important when dealing with large numbers of alarms.
Consider updating the CSS to handle different viewport sizes:
.demo-date-picker {
width: 280px;
+ @media (min-width: 768px) {
+ width: 400px;
+ }
+ @media (min-width: 1024px) {
+ width: 480px;
+ }
}📝 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.
| .demo-date-picker { | |
| width: 280px; | |
| } | |
| .demo-date-picker { | |
| width: 280px; | |
| @media (min-width: 768px) { | |
| width: 400px; | |
| } | |
| @media (min-width: 1024px) { | |
| width: 480px; | |
| } | |
| } |
| .demo-date-picker { | ||
| width: 280px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the date picker responsive.
While standardizing the width improves consistency, a fixed width of 280px might not be optimal for all screen sizes. Consider using responsive design principles.
.demo-date-picker {
- width: 280px;
+ width: 100%;
+ max-width: 280px;
+ min-width: 200px;
}📝 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.
| .demo-date-picker { | |
| width: 280px; | |
| } | |
| .demo-date-picker { | |
| width: 100%; | |
| max-width: 280px; | |
| min-width: 200px; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/date-picker/now.vue (1)
Line range hint
3-15: LGTM! Consider internationalizing the title text.The transition from inline styles to CSS classes is a good improvement for maintainability.
Consider using i18n for the Chinese text "插槽式：" and "函数式：" to support internationalization:
- <div class="title">插槽式：</div> + <div class="title">{{ $t('datePicker.slotMode') }}</div> - <div class="title">函数式：</div> + <div class="title">{{ $t('datePicker.functionMode') }}</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- examples/sites/demos/pc/app/date-picker/align-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/align.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/date-range-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/date-range.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/events-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/events.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/now-composition-api.vue (3 hunks)
- examples/sites/demos/pc/app/date-picker/now.vue (3 hunks)
- examples/sites/demos/pc/app/date-picker/timezone-composition-api.vue (1 hunks)
- packages/renderless/src/date-panel/vue.ts (1 hunks)
- packages/theme/src/date-table/index.less (1 hunks)
- packages/theme/src/month-table/index.less (1 hunks)
- packages/theme/src/month-table/vars.less (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- examples/sites/demos/pc/app/date-picker/align.vue
- examples/sites/demos/pc/app/date-picker/date-range-composition-api.vue
- examples/sites/demos/pc/app/date-picker/date-range.vue
- examples/sites/demos/pc/app/date-picker/timezone-composition-api.vue
🔇 Additional comments (10)
examples/sites/demos/pc/app/date-picker/align-composition-api.vue (2)
34-34: LGTM: Consistent width adjustment across date picker components.The width adjustment to 280px aligns with similar changes in other date picker components, maintaining consistency in the UI.
34-34: Verify width compatibility with alarm display.Since this PR is related to handling a large number of alarms, please verify that the reduced width of 280px provides sufficient space for displaying dates with multiple alarms without causing text overflow or layout issues.
Let's check for any existing alarm-related styles or components that might be affected:
examples/sites/demos/pc/app/date-picker/now-composition-api.vue (2)
Line range hint
1-55: Overall implementation looks good!The component demonstrates good Vue practices with:
- Clear separation of concerns
- Both slot-based and function-based implementations
- Properly scoped styles
- Improved maintainability by moving from inline styles to CSS classes
3-3: Verify the impact of width reduction on UI layout.The change from inline
width: 400pxto a CSS class withwidth: 280pximproves code maintainability. However, the significant width reduction (400px → 280px) might affect the layout and user experience.Please verify that:
- The date picker's content fits properly within the reduced width
- The layout remains responsive and usable on different screen sizes
Also applies to: 12-12
✅ Verification successful
Let me search for any similar date picker components to understand the width patterns across the codebase.
Width change is consistent with design system standards
The change from inline
width: 400pxto CSS class withwidth: 280pxaligns with the established pattern across the codebase. The search results show that:
- All standard date picker components use
width: 280px- Only range pickers use larger widths (360px-400px)
- The change brings consistency with other 20+ date picker examples in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other date picker width definitions to ensure consistency rg -l "demo-date-picker.*width" --type vueLength of output: 72
Script:
#!/bin/bash # Search for other date picker width definitions with ripgrep, without file type filter rg -l "demo-date-picker.*width" # Also search for style definitions containing width rg "\.demo-date-picker\s*{[^}]*width:" -A 3Length of output: 217
Script:
#!/bin/bash # Find all date picker related files fd "date-picker.*vue$" # Search for any width-related styles in these files rg "width:" $(fd "date-picker.*vue$")Length of output: 190
Script:
#!/bin/bash # Let's try a simpler search pattern for width styles rg "width:" examples/sites/demos/pc/app/date-picker/now-composition-api.vue -A 1 # Also check the mobile date picker implementation for width references cat examples/sites/demos/mobile-first/app/cell/with-date-picker.vue # Search for any style block containing demo-date-picker rg "\.demo-date-picker" -A 5Length of output: 29705
examples/sites/demos/pc/app/date-picker/events-composition-api.vue (1)
Line range hint
18-22: LGTM! Layout improvement for better space utilization.The reduction in column span from 12 to 6 for the onPick date picker creates a more compact layout while maintaining functionality. This change aligns with similar modifications in other date picker components, ensuring consistent presentation across the application.
Let's verify the consistency of this layout change across other date picker components:
✅ Verification successful
Layout changes are consistent with other date picker components
The verification confirms that the column spans are consistent across date picker components:
- Both
events-composition-api.vueandevents.vuefollow the same layout pattern- First row uses
:span="4"for individual date pickers- Second row uses
:span="6"for the daterange picker (onPick)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent column spans in date picker components # Expected: All onPick date pickers should use span="6" rg -l "tiny-date-picker.*type=\"daterange\"" | xargs rg -B 2 ":span=\""Length of output: 2265
examples/sites/demos/pc/app/date-picker/events.vue (1)
Line range hint
18-22: Verify the layout adjustment for the date range picker.The column width reduction from 12 to 6 appears reasonable for a date range picker, making it consistent with other examples. However, please ensure that:
- The date range picker has sufficient width to display both dates clearly
- The layout remains responsive on different screen sizes
✅ Verification successful
Column span adjustment aligns with existing examples
The change from span 12 to 6 for the date range picker is consistent with other examples in the codebase. Both
events.vueandevents-composition-api.vueuse:span="6"for similar date range picker components, while other date picker variants use:span="4". This standardization improves layout consistency across the demo examples.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other date range picker examples to verify consistent column spans rg -l "type=\"daterange\"" | xargs rg ":span=\"[0-9]+\""Length of output: 742
packages/theme/src/month-table/vars.less (1)
45-46: Clarify the purpose of duplicate disabled text color variables.While the new variable
--tv-MonthTable-cell-color-disabledis well-structured and follows the naming conventions, there appears to be potential duplication with the existing--tv-MonthTable-text-color-disabledvariable defined earlier in the file. Both variables seem to serve the same purpose of controlling disabled text color.Could you please clarify:
- The specific use case for each variable
- Why we need both variables
- If one should be deprecated in favor of the other
Let's check where these variables are used:
packages/theme/src/month-table/index.less (1)
67-71: LGTM: Improved disabled state stylingThe changes enhance the disabled state visibility by:
- Using a dedicated color variable
--tv-MonthTable-cell-color-disabledfor text- Maintaining consistent background color on hover with
--tv-MonthTable-bg-color-disabledThese changes improve the user interface consistency and accessibility.
packages/renderless/src/date-panel/vue.ts (1)
138-138: Verify the impact of removing the 'default' button size.The change from
'default'to''as the fallback value forbuttonSizecould affect the component's styling behavior. Please ensure:
- Components properly handle an empty string button size
- The change aligns with the design system's button size conventions
- Visual consistency is maintained across different contexts
Consider adding a comment explaining why an empty string is preferred over 'default' as the fallback value.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
The empty string fallback for buttonSize is consistent with the codebase patterns
The search results reveal that:
- In
packages/design/saas/src/date-panel/index.ts, the defaultbuttonSizeis set to 'mini'- The empty string fallback aligns with similar patterns seen in the codebase (e.g., in
examples/sites/demos/mobile-first/app/slider-button/page-turn.vue)- The component correctly handles the buttonSize prop in its template usage (
:size="state.buttonSize")🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other buttonSize usages and potential impacts # Test 1: Find other files using buttonSize to verify consistent handling echo "Searching for buttonSize usage patterns:" rg -A 2 'buttonSize' # Test 2: Check for size-related CSS classes that might depend on buttonSize echo "Checking for related CSS classes:" rg -t css -A 2 '\.(tiny|small|default|medium|large)'Length of output: 15086
packages/theme/src/date-table/index.less (1)
244-246: LGTM! Verify CSS variable definition.The new styling rule for selected dates within a range follows the existing pattern and improves visual consistency.
Let's verify the CSS variable is properly defined:
✅ Verification successful
CSS variable is properly defined and follows design system
The CSS variable
--tv-DateTable-td-bg-color-range-selectedis correctly defined invars.lesswith a fallback to#deecff. The variable follows the design system by using--tv-color-bg-active-emphasizeas its primary value.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the CSS variable is defined in vars.less rg --type less "tv-DateTable-td-bg-color-range-selected" -A 1 -B 1Length of output: 541
| .demo-date-picker { | ||
| width: 280px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CSS variables for consistent sizing.
To maintain consistent widths across all date picker components, consider defining the width value as a CSS variable.
Here's how you could improve it:
+<style>
+:root {
+ --tiny-date-picker-width: 280px;
+}
+</style>
+
<style scoped>
.demo-date-picker {
- width: 280px;
+ width: var(--tiny-date-picker-width);
}This would make it easier to maintain consistent sizing across all date picker instances and support theming.
Committable suggestion was skipped due to low confidence.
| .demo-date-picker { | ||
| width: 280px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider responsive design for different screen sizes.
While standardizing the width to 280px across components is good for consistency, consider using responsive design patterns for better mobile support.
Consider using CSS media queries for responsive behavior:
.demo-date-picker {
width: 280px;
+ @media (max-width: 320px) {
+ width: 100%;
+ max-width: 280px;
+ }
}📝 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.
| .demo-date-picker { | |
| width: 280px; | |
| } | |
| .demo-date-picker { | |
| width: 280px; | |
| @media (max-width: 320px) { | |
| width: 100%; | |
| max-width: 280px; | |
| } | |
| } |
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
Style