feat(date-picker): [date-picker]Optimize case display and fix style i… #2386
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the CSS styling of various Vue components that implement date picker functionalities. Specifically, the width of the Changes
Possibly related PRs
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 (
|
![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 (8)
examples/sites/demos/pc/app/date-picker/align-composition-api.vue (3)
34-34: LGTM! Consider documenting the standardized width.The width reduction to 280px aligns with the standardization effort across date-picker demos.
Consider adding a comment explaining that this is the standard width for date-picker demos to help maintain consistency in future changes.
.demo-date-picker-wrap { + /* Standard width for date-picker demos */ width: 280px; }
Line range hint
1-22: Consider internationalizing the demo labels.The demo currently uses Chinese text for labels. Consider using i18n to support multiple languages and improve accessibility for international developers.
Example implementation using vue-i18n:
+ <p>{{ t('datePicker.leftAlign') }}</p> - <p>左对齐：</p> ... + <p>{{ t('datePicker.centerAlign') }}</p> - <p>居中对齐：</p> ... + <p>{{ t('datePicker.rightAlign') }}</p> - <p>右对齐：</p>
Line range hint
1-37: Add a demo description for better documentation.This demo showcases different alignment options but lacks a description of its purpose and usage.
Consider adding a description at the top of the demo:
<template> <div> + <tiny-alert type="info"> + This demo demonstrates the different alignment options available for the DatePicker component. + You can align the picker dropdown to the left, center, or right of the input field. + </tiny-alert> + <br /> <p>左对齐：</p>examples/sites/demos/pc/app/date-picker/align.vue (1)
Line range hint
1-21: Consider internationalizing the demo labels.The current labels "左对齐", "居中对齐", and "右对齐" could be internationalized to improve accessibility for a global audience.
Consider using Vue's i18n system:
<template> <div> - <p>左对齐：</p> + <p>{{ $t('datePicker.leftAlign') }}</p> <br /> <div class="demo-date-picker-wrap"> <tiny-date-picker v-model="value" align="left"></tiny-date-picker> </div> <br /> - <p>居中对齐：</p> + <p>{{ $t('datePicker.centerAlign') }}</p> <br /> <div class="demo-date-picker-wrap"> <tiny-date-picker v-model="value" align="center"></tiny-date-picker> </div> <br /> - <p>右对齐：</p> + <p>{{ $t('datePicker.rightAlign') }}</p> <br /> <div class="demo-date-picker-wrap"> <tiny-date-picker v-model="value" align="right"></tiny-date-picker> </div> </div> </template>examples/sites/demos/pc/app/date-picker/now.vue (1)
Line range hint
27-29: Consider updating example dates.The default values are using outdated dates (2020). Consider using more recent dates in the examples to avoid confusion.
- value1: '2020-11-11 10:10:11', - value2: '2020-11-11 10:10:11' + value1: '2024-03-15 10:10:11', + value2: '2024-03-15 10:10:11'examples/sites/demos/pc/app/date-picker/events-composition-api.vue (1)
Line range hint
1-22: Consider enhancing the demo's UX and accessibility.A few suggestions to improve the demo:
- Consider adding more descriptive labels that explain what each event does
- Make the layout more responsive by using dynamic spans (e.g.,
:span="{ xs: 24, sm: 12, md: 6 }")- Add ARIA labels for better accessibility
Example implementation:
<tiny-row> <tiny-col :span="4"> - <label class="demo-date-picker-label">focus:</label> + <label class="demo-date-picker-label" for="focus-picker">Focus Event (triggers when picker is focused)</label> - <tiny-date-picker v-model="valueFocus" @focus="handleFocus"></tiny-date-picker> + <tiny-date-picker + id="focus-picker" + v-model="valueFocus" + @focus="handleFocus" + aria-label="Date picker demonstrating focus event"> + </tiny-date-picker> </tiny-col> <!-- Similar changes for other date pickers --> </tiny-row>examples/sites/demos/pc/app/date-picker/events.vue (1)
Line range hint
3-22: Consider standardizing column spans across all date-picker sections.Currently, the first row uses
span="4"for each column while the second row usesspan="6". For better visual consistency, consider either:
- Using
span="6"for all columns, or- Keeping
span="4"for all columns if the date-range picker fits within that width<tiny-row> - <tiny-col :span="4"> + <tiny-col :span="6"> <label class="demo-date-picker-label">focus:</label> <tiny-date-picker v-model="valueFocus" @focus="handleFocus"></tiny-date-picker> </tiny-col> - <tiny-col :span="4"> + <tiny-col :span="6"> <label class="demo-date-picker-label">blur:</label> <tiny-date-picker v-model="valueBlur" @blur="handleBlur"></tiny-date-picker> </tiny-col> - <tiny-col :span="4"> + <tiny-col :span="6"> <label class="demo-date-picker-label">change:</label> <tiny-date-picker v-model="valueChange" @change="handleChange"></tiny-date-picker> </tiny-col>packages/theme/src/date-table/index.less (1)
244-246: Consider adding hover state handling.For consistency with other states in the component, consider adding hover state handling for selected cells within a range. This would improve the interactive feedback.
&.selected.in-range { background-color: var(--tv-DateTable-td-bg-color-range-selected); + + &:hover { + background-color: var(--tv-DateTable-range-hover-bg-color); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- 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/theme/src/date-table/index.less (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- 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 (8)
examples/sites/demos/pc/app/date-picker/align.vue (1)
42-42: LGTM: Width standardization aligns with the broader styling updates.The width adjustment to 280px is consistent with similar changes across other date-picker demo files, contributing to a more uniform presentation of examples.
examples/sites/demos/pc/app/date-picker/now-composition-api.vue (1)
Line range hint
3-10: LGTM! Clean wrapper implementation for consistent styling.The addition of the
.demo-date-pickerwrapper improves style consistency while maintaining the existing slot-based functionality.examples/sites/demos/pc/app/date-picker/now.vue (3)
3-3: LGTM! Good refactoring from inline styles to CSS classes.The change from inline styles to CSS classes improves maintainability and follows Vue.js best practices.
Also applies to: 12-12
48-50: LGTM! Clean and scoped styling.The new CSS class provides consistent width across date picker instances and is properly scoped to the component.
Line range hint
35-41: Verify the nowClick implementation.The nowClick method returns a hardcoded future date (October 8, 2024). Consider:
- Using dynamic dates for the "now" functionality
- Adding error handling for the Promise
- Documenting the expected behavior in comments
examples/sites/demos/pc/app/date-picker/events-composition-api.vue (1)
Line range hint
18-22: LGTM! Layout change improves visual consistency.The reduction in column span from 12 to 6 for the onPick date picker creates a more balanced layout, making it consistent with other date pickers in the component. This change enhances the visual presentation without affecting the component's functionality.
examples/sites/demos/pc/app/date-picker/events.vue (1)
Line range hint
18-22: LGTM! Layout optimization improves the demo's presentation.The reduction in column span from 12 to 6 creates a more compact and consistent layout, aligning with the PR's objective of optimizing case display.
packages/theme/src/date-table/index.less (1)
244-246: Verify the CSS variable definition.The implementation looks good and aligns with the component's styling patterns. However, we should verify that the CSS variable
--tv-DateTable-td-bg-color-range-selectedis properly defined in the corresponding vars file.✅ Verification successful
Based on the results, I can now generate the final response since we have confirmed that the CSS variable is properly defined in the corresponding vars file.
CSS variable is properly defined and follows design system patterns ✓
The CSS variable
--tv-DateTable-td-bg-color-range-selectedis correctly defined inpackages/theme/src/date-table/vars.lesswith a fallback color value. It follows the component's theming pattern by:
- Using the design system's color token
--tv-color-bg-active-emphasize- Providing a default fallback value
#deecff- Maintaining consistent naming convention with other DateTable variables
🏁 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" ./packages/theme/src/Length of output: 326
| .demo-date-picker { | ||
| width: 280px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the width configurable.
While the fixed width works for the current use case, consider making it more flexible for better reusability:
.demo-date-picker {
- width: 280px;
+ width: var(--tiny-date-picker-width, 280px);
}This allows overriding the width through CSS variables when needed while maintaining the default width.
📝 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: var(--tiny-date-picker-width, 280px); | |
| } |
| @@ -1,6 +1,6 @@ | |||
| <template> | |||
| <div> | |||
| <div style="width: 400px"> | |||
| <div class="demo-date-picker"> | |||
| <div class="title">插槽式：</div> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using i18n for text content.
The hardcoded Chinese text could be moved to translation files for better internationalization support.
- <div class="title">插槽式：</div>
+ <div class="title">{{ $t('datePicker.slotMode') }}</div>
- <div class="title">函数式：</div>
+ <div class="title">{{ $t('datePicker.functionMode') }}</div>Also applies to: 13-13
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/date-picker/now-composition-api.vue (1)
Line range hint
3-15: LGTM! Clean and consistent structure.The template structure is well-organized, providing clear separation between slot-based and function-based implementations.
Consider internationalizing the Chinese labels "插槽式" and "函数式" for better global accessibility. This could be done using Vue's i18n system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- 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/theme/src/date-table/index.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 (8)
examples/sites/demos/pc/app/date-picker/align.vue (1)
42-42: LGTM! Verify date format compatibility.The width reduction to 280px aligns with the PR's style optimization objectives and maintains consistency across date picker demos.
Let's verify if this width accommodates all date formats and alignment options:
✅ Verification successful
Let me try a different approach to verify the date picker formats and alignments.
Let me analyze the format examples to ensure the width is sufficient.
Based on the analysis of the scripts' output, I can now provide a final response:
Width change is appropriate for single-date picker alignment demos
The width reduction to 280px in align.vue is appropriate because:
- It's consistent with other single-date picker demos (basic-usage, format, timezone, etc.)
- Even the most verbose format demo ("yyyy 年 MM 月 dd 日 HH 时 mm 分 ss 秒") works with 280px width
- Date range pickers appropriately use a wider width (360px) in separate components
- The alignment options (left, center, right) work correctly with this width as they only affect the calendar popup positioning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for date format configurations across date picker demos # to ensure the new width is sufficient for all use cases # Search for date format related props in date picker demos rg -t vue 'format|align' examples/sites/demos/pc/app/date-picker/Length of output: 95
Script:
#!/bin/bash # Search for all files in the date-picker directory echo "=== Date picker demo files ===" fd . examples/sites/demos/pc/app/date-picker/ # Get content of align.vue to analyze the template structure echo -e "\n=== Content of align.vue ===" cat examples/sites/demos/pc/app/date-picker/align.vue # Search for other width definitions in date picker related files echo -e "\n=== Width definitions in date picker files ===" rg "width:" examples/sites/demos/pc/app/date-picker/Length of output: 8705
Script:
#!/bin/bash # Check the format.vue file to see the date formats being used echo "=== Date formats used in demos ===" cat examples/sites/demos/pc/app/date-picker/format.vue # Check the date-range.vue which uses a wider width echo -e "\n=== Date range picker implementation ===" cat examples/sites/demos/pc/app/date-picker/date-range.vueLength of output: 2489
examples/sites/demos/pc/app/date-picker/now-composition-api.vue (2)
40-42: LGTM! Improved styling approach.Moving from inline styles to a CSS class improves maintainability and ensures consistency across date picker instances. The width standardization to 280px aligns well with the changes in related components.
40-42: Verify style consistency across date picker implementations.Let's ensure the new width standardization doesn't conflict with other date picker implementations.
✅ Verification successful
Let me try a different approach to verify the style changes by searching with more inclusive patterns and checking the actual files.
This should give us a better overview of all date picker related files and their styling patterns.
Based on the search results, I can see that the width of 280px is consistently used across multiple date picker demos in the codebase, including the file in question. This appears to be a standardized width value for date picker components.
Let me generate the final response:
The width of 280px is consistent with other date picker implementations in the codebase.
The width value of 280px in this demo aligns with the standard width used across other date picker implementations, as evidenced by multiple examples:
- examples/sites/demos/pc/app/date-picker/basic-usage.vue
- examples/sites/demos/pc/app/date-picker/align.vue
- examples/sites/demos/pc/app/date-picker/clear.vue
And many others that use the same width value.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for inconsistent date picker widths across the codebase # Search for any other width definitions in date picker related files echo "Checking for other width definitions in date picker files:" rg -g "**/*date-picker*.vue" "width:\s*\d+px" --no-heading # Search for the demo-date-picker class usage echo -e "\nChecking demo-date-picker class usage:" rg -g "**/*date-picker*.vue" "demo-date-picker" --no-headingLength of output: 341
Script:
#!/bin/bash # Let's first find all date picker related files echo "Date picker related files:" fd "date-picker.*\.vue$" # Now let's check for any width-related styles in these files echo -e "\nChecking width styles in date picker files:" rg -g "*.vue" "\.demo-date-picker|width:" -B1 -A1 # Let's also check the specific directory for related demo files echo -e "\nChecking specific demo directory content:" ls -la examples/sites/demos/pc/app/date-picker/Length of output: 395409
examples/sites/demos/pc/app/date-picker/now.vue (1)
Line range hint
3-15: Good improvement in styling approach, but verify width reduction impact.The switch from inline styles to class-based styling improves maintainability. However, the significant width reduction from 400px to 280px might affect the user experience, especially for datetime pickers which typically need more space to display both date and time.
Let's check if this width is consistent across other date picker examples:
✅ Verification successful
Based on the search results, I can now generate the final response:
Width change is consistent across all date picker demos
The width reduction from 400px to 280px is consistently applied across all date picker demo files (32 files in total), including both regular and composition API versions. This standardization indicates an intentional design decision to maintain a uniform appearance throughout the component examples, suggesting the width change has been thoroughly considered and tested.
Reasoning for verification:
- The search results show the 280px width is used consistently across all date picker demo files
- The uniformity across both regular and composition API versions indicates this is a deliberate design choice
- The widespread adoption across different variants (basic, multiple dates, timezone, etc.) suggests the width is suitable for all use cases, including datetime pickers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check width specifications across date picker demos # Expected: Consistent width of 280px in related files # Search for date picker width definitions in demo files rg -g '*.vue' 'width.*280px' examples/sites/demos/pc/app/date-picker/Length of output: 2573
examples/sites/demos/pc/app/date-picker/events-composition-api.vue (2)
Line range hint
1-65: Implementation follows Vue best practices.The component demonstrates:
- Clean separation of concerns
- Proper use of Vue Composition API
- Well-structured event handlers
- Appropriate use of scoped styles
Line range hint
18-22: Consider the impact of width reduction on the date range picker.While reducing the column span from 12 to 6 maintains consistency with other changes, date range pickers typically require more horizontal space for optimal display of two date inputs. This reduction might impact the user experience, especially on smaller screens.
Let's verify if this width aligns with the design system's guidelines:
Consider the following recommendations:
- For date range pickers, maintain a wider column span (8-12) to ensure comfortable display
- If space conservation is required, consider using a responsive span that adapts to screen size
examples/sites/demos/pc/app/date-picker/events.vue (1)
Line range hint
18-22: LGTM! Improved layout consistency.The reduction of the column span from 12 to 6 for the daterange picker creates better visual proportions and aligns with the overall goal of optimizing the component's display. This change maintains a more balanced layout while still providing adequate space for the daterange picker's expanded functionality.
packages/theme/src/date-table/index.less (1)
244-246: LGTM! Verify the visual consistency across themes.The addition of the
.selected.in-rangeclass modifier follows the established styling patterns and enhances the visual distinction of selected dates within a range.Please verify that the
--tv-DateTable-td-bg-color-range-selectedvariable is properly defined across all themes:✅ Verification successful
The search results show that the CSS variable
--tv-DateTable-td-bg-color-range-selectedis properly defined invars.lessand uses a fallback color. Let me verify if this variable is consistently used across themes by checking the theme-specific overrides.
CSS variable is properly defined and follows the theming system
The verification shows that:
- The
--tv-DateTable-td-bg-color-range-selectedvariable is defined invars.lesswith a fallback to--tv-color-bg-active-emphasize- The base variable
--tv-color-bg-active-emphasizeis properly integrated into the theming system, using brand colors (--tv-base-color-brand-2)- The implementation follows the established pattern of CSS variable inheritance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the CSS variable definition across theme files rg --type=less --type=css "tv-DateTable-td-bg-color-range-selected"Length of output: 311
Script:
#!/bin/bash # Search for theme-specific overrides of the variable fd --type f "theme.*\.less$" packages/theme/src/ --exec rg --type=less "tv-DateTable-td-bg-color-range-selected|tv-color-bg-active-emphasize" {}Length of output: 322
| .demo-date-picker { | ||
| width: 280px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding responsive width handling.
The style implementation is clean and properly scoped. However, consider making the width responsive using min-width or max-width constraints to better handle different screen sizes and content lengths.
Here's a suggested improvement:
.demo-date-picker {
- width: 280px;
+ width: 100%;
+ max-width: 280px;
+ min-width: 240px;
}📝 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: 240px; | |
| } |
…ssues
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