feat(date-picker): [date-picker] Fix style issues #2392
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the styling and layout of various date picker components across multiple Vue files. The width of the Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Outside diff range and nitpick comments (4)
examples/sites/demos/pc/app/date-picker/align-composition-api.vue (1)
34-34: Consider adding a comment explaining the standardized width.Since this width appears to be standardized across date picker components, it would be helpful to add a comment explaining why 280px was chosen as the standard width.
.demo-date-picker-wrap { + /* Standard width for all date picker demos to ensure consistent appearance */ width: 280px; }examples/sites/demos/pc/app/date-picker/now.vue (1)
Line range hint
37-43: Consider simplifying the Promise implementation.The
nowClickmethod usessetTimeoutwith 0ms delay, which seems unnecessary. Consider simplifying:nowClick() { - return new Promise((resolve) => { - setTimeout(() => { - resolve(new Date(2024, 9, 8, 18, 18, 18)) - }, 0) - }) + return Promise.resolve(new Date(2024, 9, 8, 18, 18, 18)) }Also, consider making the date dynamic to avoid hardcoding a specific future date that might become outdated.
examples/sites/demos/pc/app/date-picker/events-composition-api.vue (1)
Line range hint
44-54: Consider enhancing event message formatting.The event messages could be improved for better user experience and maintainability:
- Consider using i18n for internationalization support
- Format the date strings in
onPickevent to be more user-friendlyHere's a suggested improvement:
const pickerOptions = { onPick: ({ minDate, maxDate }) => { - Modal.message({ message: `触发 onPick 事件，开始日期为：${minDate}，结束日期为：${maxDate}`, status: 'info' }) + Modal.message({ + message: t('datePicker.onPickMessage', { + startDate: formatDate(minDate), + endDate: formatDate(maxDate) + }), + status: 'info' + }) } }packages/theme/src/date-table/index.less (1)
244-246: LGTM with a minor formatting suggestion.The new style rule for selected dates within a range is well-placed and uses the appropriate CSS variable pattern.
For consistency with the surrounding code, consider adding spaces after the selector and before the opening brace:
- &.selected.in-range div{ + &.selected.in-range div {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- 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)
- 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 (9)
examples/sites/demos/pc/app/date-picker/align-composition-api.vue (1)
34-34: Please verify the visual appearance with the reduced width.The width reduction from 350px to 280px aligns with similar changes across other date-picker examples. However, since this component demonstrates different alignments (left, center, right), please ensure that all alignment options display correctly with the narrower width.
✅ Verification successful
Width change is consistent across all date-picker examples
The width of 280px is consistently applied across all date-picker demo files, including both regular and composition API variants. This standardization ensures a uniform appearance throughout the component library's examples.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all date picker demo files to verify consistent width usage fd -e vue -e css --full-path "date-picker" | xargs rg "width:.+280px"Length of output: 2569
examples/sites/demos/pc/app/date-picker/now-composition-api.vue (2)
3-3: LGTM! Clean and consistent styling approach.The change from inline styles to class-based styling improves maintainability and follows Vue.js best practices.
Also applies to: 12-12
40-42: Verify the reduced width doesn't cause layout issues.The width has been reduced from 400px to 280px. While this aligns with other date picker components, please ensure:
- The date picker dropdown remains properly aligned
- All date/time content is fully visible without truncation
- The layout works well across different locales and time formats
Consider adding responsive breakpoints if this component needs to adapt to different screen sizes:
.demo-date-picker { width: 280px; + @media (max-width: 768px) { + width: 100%; + max-width: 280px; + } }examples/sites/demos/pc/app/date-picker/now.vue (2)
Line range hint
3-15: LGTM! Improved styling approach.Good refactoring from inline styles to class-based styling, which improves maintainability and follows Vue best practices.
48-50: Verify the reduced width impact.The width has been reduced from 400px to 280px, which is a significant change. Please verify that:
- This width reduction is intentional
- The date picker UI elements fit properly within the narrower container
- No text truncation or layout issues occur with different date formats
Let's check for any related width definitions or potential conflicts:
✅ Verification successful
Width change appears to be standardized across date picker examples
The width reduction from 400px to 280px aligns with the consistent pattern found across all date picker examples in the codebase. The search results show:
- Most single date picker examples use 280px width
- Date range pickers use 360px width
- Only validation-related examples use 400px width
This indicates the change is intentional and follows the established pattern in the component library's demo examples.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other width definitions that might affect the date picker rg -l "demo-date-picker" | xargs rg "width:" # Search for any related style issues or discussions gh issue list --search "date-picker width style"Length of output: 6025
examples/sites/demos/pc/app/date-picker/events-composition-api.vue (1)
Line range hint
18-22: LGTM! Layout adjustment improves the component's presentation.The reduction in column span from 12 to 6 creates a more balanced and compact layout, which is consistent with the style improvements being made across other date picker components.
examples/sites/demos/pc/app/date-picker/events.vue (1)
Line range hint
18-22: LGTM! The layout adjustment improves visual consistency.The change from
span="12"tospan="6"for the daterange picker provides a more balanced layout. This width is appropriate for displaying the date range while maintaining visual harmony with other date pickers in the row above (which usespan="4").packages/theme/src/month-table/vars.less (1)
45-46:⚠️ Potential issueConsider consolidating similar disabled text color variables.
There appear to be two variables for disabled text color:
--tv-MonthTable-text-color-disabled(line 19)--tv-MonthTable-cell-color-disabled(line 46)Having two variables for seemingly the same purpose could lead to confusion and maintenance issues.
Consider consolidating these variables by:
- Using one consistent variable name
- Updating all related styles to reference the same variable
- Documenting the specific use case if they serve different purposes
- // 文字色（禁用） - --tv-MonthTable-cell-color-disabled: var(--tv-color-text-disabled, #c2c2c2);Let's verify the usage of both variables:
packages/theme/src/month-table/index.less (1)
67-71: LGTM! Improved semantic meaning of disabled state styles.The changes properly separate the concerns between text color and background color for disabled cells, making the code more maintainable. The explicit hover state background prevents any visual changes that might incorrectly suggest interactivity.
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
Release Notes
New Features
Bug Fixes
Styling Changes