fix(time-picker): [time-picker] Style optimization #2418
Conversation
WalkthroughThe changes in this pull request focus on modifying various Vue components related to the time picker functionality. Key updates include the introduction of new props such as 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: 1
🧹 Outside diff range and nitpick comments (9)
examples/sites/demos/pc/app/time-picker/suffix-icon.vue (1)
Line range hint
7-21: Consider optimizing icon instantiation.The current implementation creates the icon instance immediately in the data property. This might lead to unnecessary instantiation as data properties are created when the component is initialized.
Consider using a computed property instead:
-data() { - return { - value1: new Date(2016, 9, 10, 18, 40), - IconMinus: IconMinus() - } -}, +data() { + return { + value1: new Date(2016, 9, 10, 18, 40) + } +}, +computed: { + IconMinus() { + return IconMinus() + } +}examples/sites/demos/pc/app/time-picker/basic-usage-composition-api.vue (2)
Line range hint
3-3: Consider internationalizing the demo labels.The Chinese text labels "滚动选择时间" and "箭头选择时间" should be internationalized for better maintainability and accessibility.
Consider using i18n:
- <p>滚动选择时间</p> + <p>{{ t('demo.timePicker.scrollSelect') }}</p> - <p>箭头选择时间</p> + <p>{{ t('demo.timePicker.arrowSelect') }}</p>Also applies to: 8-8
Line range hint
18-18: Document the purpose of the demo date value.Consider adding a comment explaining why this specific date (2016-10-10 18:40) was chosen for the demo.
+// Demo date set to 2016-10-10 18:40 to showcase a specific time format const value1 = ref(new Date(2016, 9, 10, 18, 40))examples/sites/demos/pc/app/time-picker/basic-usage.vue (2)
Line range hint
1-13: Consider internationalizing the text labels.The hardcoded Chinese text labels
"滚动选择时间"and"箭头选择时间"should be internationalized for better maintainability and global accessibility.Consider using a translation system or i18n keys:
<template> <div> - <p>滚动选择时间</p> + <p>{{ t('time.scrollSelect') }}</p> <div class="demo-date-picker-wrap"> <tiny-time-picker v-model="value1"></tiny-time-picker> </div> - <p>箭头选择时间</p> + <p>{{ t('time.arrowSelect') }}</p> <div class="demo-date-picker-wrap"> <tiny-time-picker v-model="value1" arrow-control></tiny-time-picker> </div> </div> </template>
Line range hint
15-26: Consider using a more readable date format.While the current date initialization works, using a more explicit format would improve readability.
Consider using ISO string or Date constructor with a string:
data() { return { - value1: new Date(2016, 9, 10, 18, 40) + value1: new Date('2016-10-10T18:40:00') } }examples/sites/demos/pc/app/time-picker/placeholder.vue (1)
Line range hint
35-44: Consider adding responsive width adjustments.While the fixed width works well for desktop, consider making the width responsive for better mobile experience.
.demo-date-picker-wrap { width: 280px; + @media (max-width: 480px) { + width: 100%; + max-width: 280px; + } }examples/sites/demos/pc/app/time-picker/clearable-composition-api.vue (1)
Line range hint
4-14: Separate v-model bindings needed for each time picker instance.Currently, all three time picker instances share the same
value1binding, which means changing one will update all three simultaneously. This makes it difficult to demonstrate their individual behaviors clearly.Consider using separate bindings for each instance:
- <tiny-time-picker v-model="value1"></tiny-time-picker> + <tiny-time-picker v-model="value1"></tiny-time-picker> - <tiny-time-picker v-model="value1" :clearable="false"></tiny-time-picker> + <tiny-time-picker v-model="value2" :clearable="false"></tiny-time-picker> - <tiny-time-picker v-model="value1" :clear-icon="IconError"></tiny-time-picker> + <tiny-time-picker v-model="value3" :clear-icon="IconError"></tiny-time-picker>And in the script:
const value1 = ref(new Date(2016, 9, 10, 18, 40)) const value2 = ref(new Date(2016, 9, 10, 18, 40)) const value3 = ref(new Date(2016, 9, 10, 18, 40))examples/sites/demos/pc/app/time-picker/clearable.vue (1)
Line range hint
19-33: Consider using a dynamic default date.While the implementation is correct, using a hardcoded date (
new Date(2016, 9, 10, 18, 40)) as the default value might not be the most maintainable approach.Consider using the current date or a relative date:
- value1: new Date(2016, 9, 10, 18, 40), + value1: new Date(new Date().setHours(18, 40, 0, 0)),examples/sites/demos/pc/app/time-picker/format.vue (1)
Line range hint
1-24: Consider internationalizing the Chinese text.While not directly related to the current style changes, consider internationalizing the Chinese text in the template for better global accessibility. This would make the examples more accessible to non-Chinese speaking users.
Example approach:
- <p>时间输入框中显示的格式：</p> + <p>{{ t('timePicker.displayFormat') }}</p> - <p>选中值的格式：</p> + <p>{{ t('timePicker.selectedFormat') }}</p> - <p>下拉框中显示的格式：</p> + <p>{{ t('timePicker.dropdownFormat') }}</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (30)
- examples/sites/demos/pc/app/time-picker/basic-usage-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/basic-usage.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/clearable-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/clearable.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/default-value-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/default-value.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/disabled-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/disabled.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/editable-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/editable.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/event-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/event.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/format-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/format.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/is-range-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/time-picker/is-range.vue (2 hunks)
- examples/sites/demos/pc/app/time-picker/name-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/name.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/picker-options-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/picker-options.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/placeholder-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/placeholder.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/popper-class-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/popper-class.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/size.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/step-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/step.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/suffix-icon-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/suffix-icon.vue (1 hunks)
✅ Files skipped from review due to trivial changes (16)
- examples/sites/demos/pc/app/time-picker/default-value.vue
- examples/sites/demos/pc/app/time-picker/disabled-composition-api.vue
- examples/sites/demos/pc/app/time-picker/disabled.vue
- examples/sites/demos/pc/app/time-picker/editable-composition-api.vue
- examples/sites/demos/pc/app/time-picker/editable.vue
- examples/sites/demos/pc/app/time-picker/event-composition-api.vue
- examples/sites/demos/pc/app/time-picker/event.vue
- examples/sites/demos/pc/app/time-picker/format-composition-api.vue
- examples/sites/demos/pc/app/time-picker/is-range.vue
- examples/sites/demos/pc/app/time-picker/name-composition-api.vue
- examples/sites/demos/pc/app/time-picker/name.vue
- examples/sites/demos/pc/app/time-picker/picker-options.vue
- examples/sites/demos/pc/app/time-picker/popper-class-composition-api.vue
- examples/sites/demos/pc/app/time-picker/size-composition-api.vue
- examples/sites/demos/pc/app/time-picker/size.vue
- examples/sites/demos/pc/app/time-picker/step-composition-api.vue
🔇 Additional comments (23)
examples/sites/demos/pc/app/time-picker/suffix-icon-composition-api.vue (3)
Line range hint
1-5: LGTM! Clean and well-structured template.The template follows Vue best practices with proper component usage and binding.
Line range hint
7-13: LGTM! Modern script setup implementation.The script section uses recommended practices with clear initialization and proper dependency management.
17-19: Verify consistent width changes across time picker components.The width increase to 280px appears to be part of a broader standardization effort. Let's verify this change is consistent across related components.
✅ Verification successful
Width changes are consistent across all time picker components
The verification confirms that all time picker demo components consistently use a width of 280px for the
.demo-date-picker-wrapclass, showing successful standardization across the entire time picker component set.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify width consistency across time picker demo files # Expected: All time picker demos should use the same width value # Search for width declarations in time picker demo files echo "Checking width declarations in time picker demo files:" rg -l "demo-date-picker-wrap" examples/sites/demos/pc/app/time-picker/ | xargs rg "width:"Length of output: 2213
examples/sites/demos/pc/app/time-picker/picker-options-composition-api.vue (1)
20-20: LGTM! Width standardization aligns with the style optimization goal.The width adjustment to 280px is consistent with similar changes across other time-picker demo files, contributing to a more uniform and optimized presentation.
examples/sites/demos/pc/app/time-picker/is-range-composition-api.vue (1)
2-4: LGTM! Good practice adding a wrapper class.The addition of a wrapper class helps maintain consistent styling across time picker components.
examples/sites/demos/pc/app/time-picker/popper-class.vue (1)
24-24: LGTM! Width adjustment aligns with style optimization goals.The increased width provides more space for the time picker component, which should improve the user experience and visual presentation.
Let's verify this width change is consistent across other time-picker demo files:
✅ Verification successful
Width change is consistent across time-picker demos
The width change to 280px is consistently applied across all regular time-picker demo files, with only range picker demos using 360px (which is appropriate for their dual-input nature). This confirms the style optimization is properly implemented across the component demos.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent width values across time-picker demo files # Expect: All demo files should use the same width value rg -g '*.vue' -l 'demo-date-picker-wrap.*width' examples/sites/demos/pc/app/time-picker/ rg -g '*.vue' 'demo-date-picker-wrap.*width:.*px' examples/sites/demos/pc/app/time-picker/Length of output: 183
Script:
#!/bin/bash # Let's try a different approach to check width values in time-picker demo files # First, let's find all style blocks with width declarations rg -g '*.vue' -A 2 -B 2 'width:.*px' examples/sites/demos/pc/app/time-picker/ # Also check for any class or style definitions that might affect width rg -g '*.vue' ':style="|style="' examples/sites/demos/pc/app/time-picker/Length of output: 11490
examples/sites/demos/pc/app/time-picker/suffix-icon.vue (2)
Line range hint
1-5: LGTM! Template structure follows Vue best practices.The template correctly implements the time picker with proper v-model binding and suffix icon integration.
25-28: LGTM! Style change aligns with PR objectives.The width adjustment to 280px is consistent with the style optimization goals and matches the changes in other time picker components.
examples/sites/demos/pc/app/time-picker/basic-usage-composition-api.vue (1)
24-24: LGTM! Verify width consistency across time picker demos.The width adjustment to 280px aligns with the layout standardization effort.
✅ Verification successful
Width is consistent across time picker demos
The verification shows that all
.demo-date-picker-wrapclasses consistently usewidth: 280pxacross the time picker demo files. There are only two exceptions with different class names:
.demo-date-picker-optionswhich also uses 280px.demo-time-picker-wrapwhich uses 360px for a different component typeThe width adjustment to 280px in this file aligns perfectly with the standardized width used across other time picker demos.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent width styling across time picker demos # Expected: All time picker demos should use the same width value # Search for width declarations in time-picker demo files rg -g '*/time-picker/*.vue' 'width:.*px' examples/sites/demos/pc/app/Length of output: 71
Script:
#!/bin/bash # Let's try a different approach to find width styles in time picker demos # First, let's find all time picker demo files fd -e vue . examples/sites/demos/pc/app/time-picker/ -x cat {} | grep -B2 -A2 "width:"Length of output: 2193
examples/sites/demos/pc/app/time-picker/step.vue (1)
30-30: LGTM: Width adjustment aligns with style optimization goals.The increased width provides better visual presentation and maintains consistency with similar changes across other time-picker demo files.
examples/sites/demos/pc/app/time-picker/basic-usage.vue (1)
Line range hint
31-37: LGTM! Width adjustment improves component layout.The increased width from 200px to 280px provides better spacing for the time picker component, improving its visual presentation and usability.
examples/sites/demos/pc/app/time-picker/placeholder-composition-api.vue (1)
30-30: LGTM! Verify width consistency across time picker components.The width increase to 280px appears appropriate for better time format display and placeholder text accommodation.
Let's verify this width is consistently applied across other time picker components:
✅ Verification successful
Width change is consistent with the component library's standards
The width of 280px is consistently applied across all time-picker components in the codebase, making this change align with the established pattern. The search results show that:
- All time-picker components use the same 280px width for
.demo-date-picker-wrap- This width is also consistently used in related date-picker components
- Only a few specific date-picker variants (like date-range and validate-event) use different widths (360px and 400px) for their special use cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the width value is consistent across time picker components # Expected: All time picker components should use width: 280px # Search for demo-date-picker-wrap class width definitions rg -g '*.vue' 'demo-date-picker-wrap.*width:'Length of output: 47
Script:
#!/bin/bash # Let's try a different approach to find width definitions in time-picker related files # First, find all time-picker related Vue files fd -e vue time-picker # Then search for width definitions in these files rg -g '*.vue' '\.demo-date-picker-wrap\s*{[^}]*width:' -A 2Length of output: 233
Script:
#!/bin/bash # Let's fix the regex and try again # First, find time-picker related Vue files fd -e vue time-picker # Then search for width definitions with a simpler pattern rg -g '*.vue' '\.demo-date-picker-wrap' -B 1 -A 3Length of output: 24408
examples/sites/demos/pc/app/time-picker/placeholder.vue (3)
Line range hint
1-17: LGTM! Clean template structure with clear user guidance.The template follows Vue best practices with:
- Clear section separation for single time and time range pickers
- Descriptive Chinese placeholders that improve user experience
- Consistent use of the demo-date-picker-wrap class for layout
Line range hint
19-33: LGTM! Well-structured component implementation.The script section demonstrates clean code with:
- Proper component registration
- Clear data initialization for both single and range values
38-38: Width adjustment aligns with PR objectives.The increased width from 200px to 280px provides better visual space for the time picker, especially considering the Chinese placeholder text which typically requires more horizontal space.
Let's verify if this width change is consistent across other time-picker demos:
✅ Verification successful
Width change is consistently applied across all time-picker demos
The verification confirms that all 26 time-picker demo files consistently use the width value of 280px, showing a uniform implementation of the style optimization mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check width consistency across time-picker demos # Expect: All time-picker demos should use the same width value rg -l "demo-date-picker-wrap" examples/sites/demos/pc/app/time-picker/ | xargs rg "width:"Length of output: 2094
examples/sites/demos/pc/app/time-picker/default-value-composition-api.vue (3)
Line range hint
1-13: LGTM! Template structure is well-organized.The template follows Vue best practices with proper two-way binding and clear separation between single and range time picker demonstrations.
Line range hint
15-23: LGTM! Script implementation is clean and follows Vue Composition API best practices.The reactive references are properly initialized and the component is correctly imported.
28-28: LGTM! Width adjustment improves the time picker's visual presentation.The increased width provides better spacing for the time picker component. This change aligns with the PR's style optimization objective.
Let's verify that this width change is consistent across other time picker demos:
examples/sites/demos/pc/app/time-picker/clearable-composition-api.vue (2)
Line range hint
18-24: LGTM! Clean script setup implementation.The script setup follows Vue 3 best practices with proper imports and reactive state initialization.
29-29: Width adjustment looks good, verify consistency across components.The width increase to 280px aligns with the style optimization objective.
Let's verify this width is consistently applied across other time-picker components:
✅ Verification successful
Width is consistently applied across time-picker components
The width of 280px is uniformly applied across all time-picker components, with the only exception being the range picker variants (
is-range.vueandis-range-composition-api.vue) which appropriately use 360px due to their dual-input nature. This confirms the style change is part of a consistent design pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent width styling across time-picker components # Expected: All time-picker demo components should use the same width # Search for width declarations in time-picker demo files rg -g '*/time-picker/*.vue' 'width:.*px' --no-headingLength of output: 204
Script:
#!/bin/bash # Let's try a different approach using fd to first find the files # and then search for width declarations in them # Find all Vue files in time-picker directories fd -g '*time-picker/*.vue' | while read -r file; do echo "=== $file ===" rg 'width:.*px' "$file" || true doneLength of output: 43
Script:
#!/bin/bash # Let's try a more specific path based on the file under review # Since we know the file is in examples/sites/demos/pc/app/time-picker/ # let's search in that directory cd examples/sites/demos/pc/app/time-picker/ 2>/dev/null && \ echo "=== Time Picker Directory Contents ===" && \ ls -la *.vue && \ echo -e "\n=== Width Declarations ===" && \ grep -H "width:" *.vue || \ echo "Failed to access directory or find files"Length of output: 4761
examples/sites/demos/pc/app/time-picker/clearable.vue (2)
Line range hint
1-17: LGTM! Well-structured template with clear examples.The template effectively demonstrates three distinct use cases for the time-picker's clearable functionality, with clear progressive enhancement from basic to advanced usage.
36-38: Verify consistent width across all time-picker components.The width adjustment to 280px aligns with the style optimization objective.
Let's verify this width is consistently applied across all time-picker components:
✅ Verification successful
Width adjustments are inconsistent across time-picker components
The verification reveals inconsistent width values across time-picker components:
- Regular time picker components use
width: 280px(clearable.vue, basic-usage.vue)- Range time picker uses
width: 360px(is-range.vue)This difference is actually expected as range pickers need more space to display two time inputs. The width adjustment to 280px is correctly applied to all non-range time picker demos, while range picker appropriately uses a wider container.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent width styling across time-picker components # Expected: All time-picker demo components should use the same width # Search for width declarations in time-picker demo files rg -g '*/time-picker/*.vue' 'width:.*px' examples/sites/demos/pc/app/Length of output: 71
Script:
#!/bin/bash # Let's try a different approach to find width-related styles in time-picker components # First, let's check all CSS/SCSS classes in time-picker Vue files rg -g '*/time-picker/*.vue' '\.demo-date-picker-wrap\s*{[^}]*}' examples/sites/demos/pc/app/ # Also check for style blocks with width property rg -g '*/time-picker/*.vue' '<style[^>]*>[\s\S]*?width:.*?px[\s\S]*?</style>' examples/sites/demos/pc/app/ # Additionally, check for any inline width styles rg -g '*/time-picker/*.vue' ':style="[^"]*width[^"]*"' examples/sites/demos/pc/app/Length of output: 432
Script:
#!/bin/bash # Let's try simpler patterns to find width-related styles # First, list all time-picker Vue files fd '.vue$' examples/sites/demos/pc/app/time-picker/ # Then check each file's style section rg -g '*/time-picker/*.vue' '<style' -A 10 examples/sites/demos/pc/app/Length of output: 1973
Script:
#!/bin/bash # Let's check the content of a few representative time-picker files to see their styling cat examples/sites/demos/pc/app/time-picker/clearable.vue cat examples/sites/demos/pc/app/time-picker/basic-usage.vue cat examples/sites/demos/pc/app/time-picker/is-range.vueLength of output: 2265
examples/sites/demos/pc/app/time-picker/format.vue (1)
53-53: LGTM: Width adjustment improves time-picker presentation.The increased width from 200px to 280px provides better spacing for various time formats, especially those with AM/PM indicators. This change is consistently applied across multiple example files, ensuring a unified appearance throughout the documentation.
| <style scoped lang="less"> | ||
| .demo-time-picker-wrap { | ||
| width: 360px; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CSS custom properties for consistent widths.
While the width of 360px is appropriate for a range time picker (accommodating two time inputs), consider defining it as a CSS custom property (CSS variable) in a shared style file. This would make it easier to maintain consistent widths across all time picker variants.
<style scoped lang="less">
.demo-time-picker-wrap {
- width: 360px;
+ width: var(--tiny-time-picker-range-width, 360px);
}
</style>📝 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.
| <style scoped lang="less"> | |
| .demo-time-picker-wrap { | |
| width: 360px; | |
| } | |
| </style> | |
| <style scoped lang="less"> | |
| .demo-time-picker-wrap { | |
| width: var(--tiny-time-picker-range-width, 360px); | |
| } | |
| </style> |
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
Improvements
Bug Fixes
These enhancements aim to improve the user experience and flexibility of the time picker component.