feat(date-picker): [date-picker] Adapting to the SMB themem #2205
Conversation
WalkthroughThe pull request introduces various modifications across multiple Vue.js components, primarily focusing on simplifying the date and time picker functionalities. Key changes include the removal 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: 6
🧹 Outside diff range and nitpick comments (20)
packages/design/saas/src/date-panel/index.ts (1)
1-6: LGTM! Consider enhancing with types and documentation.The changes align well with the PR objective of adapting the date-picker to the SMB theme. The default export sets up the initial state for button styling in the date-panel component.
To further improve this code:
Add documentation to explain the purpose and usage of this file. For example:
/** * Default configuration for the date-panel component. * This sets up the initial state for button styling within the date-picker. */Consider adding type definitions for improved type safety:
interface DatePanelState { buttonType: 'primary' | 'secondary' | 'text'; buttonSize: 'mini' | 'small' | 'medium' | 'large'; } const state: DatePanelState = { buttonType: 'primary', buttonSize: 'mini' }; export default { state };The file name
index.tsis generic. Consider renaming it to better reflect its role, e.g.,datePanelConfig.tsordatePanelDefaults.ts.examples/sites/demos/pc/app/time-picker/is-range-composition-api.vue (1)
Line range hint
9-17: Consider making initial time values more flexibleThe current implementation uses hardcoded initial values for
startTimeandendTime. To improve the reusability and flexibility of this component, consider the following suggestions:
- Accept initial values as props, allowing parent components to set them.
- Use the current date and time as default values if no props are provided.
Here's an example of how you could refactor this:
<script setup> -import { ref } from 'vue' +import { ref, computed } from 'vue' import { TimePicker as TinyTimePicker } from '@opentiny/vue' -const startTime = new Date(2016, 9, 10, 18, 40) -const endTime = new Date(2016, 9, 10, 18, 50) +const props = defineProps({ + initialStartTime: { + type: Date, + default: () => new Date() + }, + initialEndTime: { + type: Date, + default: () => { + const date = new Date(); + date.setMinutes(date.getMinutes() + 10); + return date; + } + } +}) -const value1 = ref([startTime, endTime]) +const value1 = ref([props.initialStartTime, props.initialEndTime]) </script>This change allows the component to be more flexible while still providing sensible defaults.
examples/sites/demos/pc/app/date-picker/label-inside.vue (1)
29-32: Consider adding explanatory comments.The changes align well with the PR objective of adapting the date-picker to the SMB theme. However, to improve code maintainability and clarity, consider adding comments explaining the reasoning behind these specific adjustments.
For example:
:deep(.tiny-date-editor--daterange.tiny-input__inner) { /* Increased width to accommodate SMB theme requirements */ width: 360px; } :deep(.tiny-range__close-icon) { /* Adjusted position to align with SMB theme layout */ right: -4px; }This will help other developers understand the purpose of these changes in the context of the SMB theme adaptation.
packages/design/saas/index.ts (1)
43-44: Consider alphabetizing the components object for improved maintainability.The addition of
DatePanelto the exported components is correct. However, to improve readability and maintainability, consider alphabetizing the entirecomponentsobject. This would make it easier to locate specific components and maintain consistency as new components are added in the future.Here's a suggested refactor:
export default { name: 'saas', version, components: { Alert, Badge, CollapseItem, + DatePanel, + DateRange, Drawer, Dropdown, DropdownMenu, DropdownItem, Form, Grid, - Switch, - Select, - Popover, - Loading, Input, - DateRange, + Loading, Pager, + Popeditor, + Popover, + Select, + Switch, - DialogBox, - Popeditor, - DatePanel + DialogBox } }examples/sites/demos/pc/app/time-picker/size.spec.ts (2)
17-17: Approve change and update TODO commentThe adjustment to 40px for the medium-sized range time picker aligns it with the single time picker, addressing the inconsistency mentioned in the TODO comment. This change improves overall UI consistency.
Consider removing or updating the TODO comment on line 16, as the inconsistency has been addressed:
- // TINY-TODO: 日期和日期范围的输入框高度不一致 + // Heights for date and date range inputs are now consistent
22-23: Approve change and update TODO commentThe adjustment to 28px for both single and range time pickers in small size improves consistency and addresses the issue mentioned in the TODO comment. This change aligns well with the PR objective of adapting to the SMB theme.
Consider removing or updating the TODO comment on line 21, as the inconsistency has been addressed:
- // TINY-TODO: 日期和日期范围的输入框高度不一致 + // Heights for date and date range inputs are now consistentexamples/sites/demos/pc/app/time-picker/size.vue (1)
6-7: Approved: Improved logical order of size optionsThe reordering of the radio buttons for size options (medium, default, small, mini) creates a more intuitive descending size sequence. This change enhances user understanding and ease of selection without affecting the component's functionality.
Consider adding a comment above the
<tiny-radio-group>explaining the rationale behind this specific order of sizes. For example:<!-- Size options are ordered from largest to smallest for intuitive selection --> <tiny-radio-group v-model="radioValue" size="mini" class="demo-time-picker__switch-size"> ... </tiny-radio-group>examples/sites/demos/pc/app/date-picker/size-composition-api.vue (1)
Line range hint
1-63: Consider updating documentation for these UI changes.While the changes made are relatively minor, it would be beneficial to update any relevant documentation or add comments in the code to explain:
- The new order of size options in the radio button group.
- The reason for the specific width increase of the date range picker.
This will help maintain clarity for future developers working on this component.
examples/sites/demos/pc/app/date-picker/size.vue (1)
Line range hint
1-71: Summary of changes and request for additional contextThe changes in this file include reordering the size options for the date picker and increasing the width of the date range picker. While these changes seem to improve the component's usability, it would be helpful to have more context about the SMB theme adaptation mentioned in the PR objectives.
Could you provide more details on the specific requirements of the SMB theme? This would help us better understand how these changes contribute to the theme adaptation and if there are any other modifications needed to fully align with the SMB theme guidelines.
packages/theme-saas/src/picker/index.less (2)
109-109: Approved with suggestion: Consider consistent use of utility classes.The changes to
.@{range-prefix-cls}__close-iconimprove the layout and align with the SMB theme. The use of utility classes for width, height, and margin is good for consistency. However, I noticed a mix of utility classes and traditional CSS properties in this block.Consider refactoring the entire block to use utility classes consistently. This could improve maintainability and make it easier to adapt to theme changes in the future. For example:
.@{range-prefix-cls}__close-icon { @apply absolute right-1.5 text-base w-4 h-5 mr-6; @apply flex justify-center items-center; float: right; @apply leading-8; }This approach would make the styling more consistent with the newly added utility classes.
Also applies to: 116-117
Incomplete removal of
.baseCleariconclass.The
.baseCleariconclass is still present in multiple files:
packages/theme/src/picker/index.lesspackages/theme-saas/src/picker/index.less(withinsvg.baseClearicon:hover)Please ensure that all instances of
.baseCleariconare removed or refactored appropriately to prevent any unintended behaviors.🔗 Analysis chain
Line range hint
1-285: Please clarify the removal of.baseCleariconclass.The AI summary mentions that the
.baseCleariconclass has been removed from the.@{input-prefix-cls}__iconclass. While this simplifies the CSS structure, it's not clear if this removal was intentional or if it might have unintended consequences.Could you please confirm if the removal of
.baseCleariconwas intentional? If so, please ensure that the hover behavior for the clear icon is still working as expected. You may want to run the following command to check for any remaining usage of.baseCleariconin the codebase:If the removal was unintentional, please consider adding it back to maintain the existing hover behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of .baseClearicon rg --type less --type css '.baseClearicon'Length of output: 228
packages/theme/src/picker/index.less (3)
119-122: LGTM: New close box stylingThe addition of the
.@{range-prefix-cls}__close-boxclass looks good. It creates a container for the close icon with appropriate positioning and layering.One small suggestion: Consider using a variable for the z-index value to maintain consistency across the theme.
Here's a suggested minor improvement:
- z-index: 5; + z-index: var(--ti-date-picker-close-box-z-index, 5);Don't forget to define the
--ti-date-picker-close-box-z-indexvariable in your theme variables file.
127-132: Close icon styling improvementsThe changes to the
.@{range-prefix-cls}__close-iconclass look good overall. They provide more specific styling for the close icon. However, there are a couple of points to consider:
The absolute positioning with
right: -20pxplaces the icon outside its container. Ensure this doesn't cause any layout issues or unintended overlaps.The hardcoded white background color might cause visibility issues on light backgrounds. Consider using a theme variable that can adapt to different background contexts.
Here are some suggested improvements:
width: 20px; height: 18px; position: absolute; right: -20px; - background-color: #FFFFFF; + background-color: var(--ti-date-picker-close-icon-bg, #FFFFFF); margin-top: -1px;Also, consider adding a subtle box-shadow or border to ensure visibility against all background colors:
box-shadow: 0 0 0 1px var(--ti-date-picker-close-icon-border-color, rgba(0,0,0,0.1));Don't forget to define the new variables in your theme variables file.
151-158: Consider more flexible width and LGTM on alignmentThe change to center-align the items within the range editor looks good and aligns with the SMB theme objectives.
However, setting a fixed width of 360px might limit flexibility in different contexts.
Consider using a more flexible approach:
.@{range-editor-prefix-cls}.@{input-prefix-cls}__inner { - width: 360px; + width: var(--ti-date-range-editor-width, 360px); + max-width: 100%; }This change allows the width to be customized via a theme variable while ensuring it doesn't overflow its container. Don't forget to define the
--ti-date-range-editor-widthvariable in your theme variables file.packages/vue/src/picker/src/pc.vue (1)
120-120: Consider removing duplicate class.The class
tiny-input__iconis repeated twice on this element. This doesn't affect functionality but it's unnecessary and could be confusing for maintainers.Consider removing the duplicate class:
- <i class="tiny-input__icon tiny-input__icon"> + <i class="tiny-input__icon">packages/renderless/src/date-panel/vue.ts (2)
92-92: LGTM! Consider adding type annotation fordesignConfig.The addition of
designConfigparameter is a good approach for customization and aligns with the PR objective.Consider adding a type annotation for
designConfigto improve code readability and maintainability. For example:const initState = ({ reactive, computed, api, i18n, designConfig }: { /* existing types */, designConfig: DesignConfig }) => { // ... }Replace
DesignConfigwith the appropriate type or interface for your design configuration.
235-239: LGTM! Consider restructuring for improved readability.The addition of
designConfigparameter is consistent with the changes in theinitStatefunction and supports the PR objective.To improve readability, consider restructuring the function signature. For example:
export const renderless = ( props, context: { computed: any; reactive: any; watch: any; nextTick: any }, dependencies: { t: any; emit: any; vm: any; i18n: any; designConfig: any } ) => { const { computed, reactive, watch, nextTick } = context; const { t, emit: $emit, vm, i18n, designConfig } = dependencies; // ... rest of the function }This structure groups related parameters and makes the function signature easier to read and maintain.
packages/vue/src/date-panel/src/pc.vue (2)
212-216: Approve footer layout enhancement with a suggestion for improved readability.The addition of dynamic
justifyContentstyling based onstate.selectionModeimproves the flexibility of the footer layout. This change allows for better alignment of footer elements in different selection modes.Consider extracting the condition into a computed property for improved readability:
<script setup> // ... const isSpaceBetweenJustified = computed(() => !['dates', 'years'].includes(state.selectionMode)) </script> <template> <div class="tiny-picker-panel__footer" v-show="state.isShowFooter" :style="{ justifyContent: isSpaceBetweenJustified ? 'space-between' : 'right' }" > <!-- ... --> </div> </template>This change would make the template more readable and the logic easier to understand and maintain.
Line range hint
219-233: Approve button enhancements with a suggestion for consistency.The modifications to the button components improve the flexibility and customization options of the date picker. The conditional visibility of the "Now" button and the dynamic bindings for
sizeandtypealign well with the PR objective of adapting to the SMB theme.For consistency, consider applying the same conditional visibility to the "Confirm" button as well:
<tiny-button v-show="!['dates', 'years'].includes(state.selectionMode)" :type="state.buttonType" :size="state.buttonSize" class="tiny-picker-panel__link-btn" @click="confirm" > {{ t('ui.datepicker.confirm') }} </tiny-button>This change would ensure that both buttons have consistent visibility rules, which might be beneficial for certain selection modes.
packages/vue/src/date-range/src/pc.vue (1)
Line range hint
217-225: LGTM! Consider applying consistent styling to both buttons.The removal of the
sizeattribute from both buttons aligns with the PR objective of adapting to the SMB theme. The functionality remains intact, and the internationalization support is maintained.For consistency, consider applying the same styling approach to both buttons. Currently, the "Clear" button uses
type="text", while the "Confirm" button uses dynamic binding for itstypeattribute. You might want to use dynamic binding for both buttons to ensure a consistent look and feel. For example:<tiny-button :type="state.clearButtonProps.type || 'text'" class="tiny-picker-panel__link-btn" @click="handleClear" > {{ t('ui.datepicker.clear') }} </tiny-button>This change would allow for easier theming and maintenance in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
- examples/sites/demos/pc/app/date-picker/date-range-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/date-picker/date-range.vue (0 hunks)
- examples/sites/demos/pc/app/date-picker/default-value-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/default-value.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/disabled-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/disabled.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/label-inside-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/label-inside.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/shortcuts-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/shortcuts.vue (1 hunks)
- examples/sites/demos/pc/app/date-picker/size-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/date-picker/size.vue (2 hunks)
- examples/sites/demos/pc/app/time-picker/is-range-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/is-range.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/time-picker/size.spec.ts (1 hunks)
- examples/sites/demos/pc/app/time-picker/size.vue (1 hunks)
- packages/design/saas/index.ts (2 hunks)
- packages/design/saas/src/date-panel/index.ts (1 hunks)
- packages/renderless/src/date-panel/vue.ts (3 hunks)
- packages/theme-saas/src/picker/index.less (2 hunks)
- packages/theme/src/date-panel/index.less (0 hunks)
- packages/theme/src/picker/index.less (2 hunks)
- packages/theme/src/picker/vars.less (0 hunks)
- packages/vue/src/date-panel/src/pc.vue (2 hunks)
- packages/vue/src/date-range/src/pc.vue (1 hunks)
- packages/vue/src/picker/src/pc.vue (1 hunks)
💤 Files with no reviewable changes (4)
- examples/sites/demos/pc/app/date-picker/date-range-composition-api.vue
- examples/sites/demos/pc/app/date-picker/date-range.vue
- packages/theme/src/date-panel/index.less
- packages/theme/src/picker/vars.less
✅ Files skipped from review due to trivial changes (2)
- examples/sites/demos/pc/app/date-picker/shortcuts-composition-api.vue
- examples/sites/demos/pc/app/date-picker/shortcuts.vue
🔇 Additional comments (28)
examples/sites/demos/pc/app/time-picker/is-range-composition-api.vue (1)
Line range hint
1-18: Overall assessment: Changes align with PR objectivesThe modifications to this file appear to be in line with the PR objectives of adapting the date-picker to the SMB theme. The removal of the explicit
range-separatorprop likely contributes to this goal by standardizing the component's appearance.However, to ensure a smooth transition:
- Verify that the default separator aligns with the SMB theme requirements.
- Test the component in different locales to confirm that the user experience remains positive across languages.
- Consider the suggested refactoring to improve the component's flexibility.
These changes, combined with proper testing and verification, should successfully adapt the time-picker to the SMB theme while maintaining or improving its functionality.
examples/sites/demos/pc/app/time-picker/is-range.vue (1)
3-3: Clarify the rationale behind removing therange-separatorattribute.The removal of the
range-separatorattribute from the<tiny-time-picker>component might affect how the time range is displayed to users. While this change could be part of adapting to the SMB theme, it's not immediately clear how it relates to the PR objectives.Could you please provide more context on:
- How this change specifically contributes to adapting the date-picker to the SMB theme?
- What will be used as the default separator now, and how does it align with the SMB theme guidelines?
Additionally, I recommend:
- Documenting this change in the component's documentation or release notes, as it might affect the behavior that users expect.
- Verifying that this change is consistently applied across all instances of the range time picker in the SMB theme implementation.
To ensure consistency, let's check for other occurrences of the
range-separatorattribute in time picker components:This will help us identify if there are any inconsistencies in the implementation across the codebase.
✅ Verification successful
Verification Successful: No additional instances of
range-separatorfound.The removal of the
range-separatorattribute from the<tiny-time-picker>component inis-range.vueappears to be an isolated change and does not affect other parts of the codebase. This aligns with the PR objectives of adapting to the SMB theme without introducing inconsistencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of range-separator in time picker components rg --type vue 'tiny-time-picker.*range-separator' .Length of output: 81
Script:
#!/bin/bash # Search for other occurrences of range-separator in tiny-time-picker components within .vue files rg 'tiny-time-picker.*range-separator' --include '*.vue' .Length of output: 502
Script:
#!/bin/bash # Find all .vue files and search for range-separator in tiny-time-picker components find . -type f -name '*.vue' -exec grep -H 'tiny-time-picker.*range-separator' {} +Length of output: 87
examples/sites/demos/pc/app/date-picker/label-inside-composition-api.vue (2)
21-24: Summary: Style adjustments for SMB theme adaptationThe changes to the date picker styling align well with the PR objective of adapting to the SMB theme. The increased width of the date range picker and the repositioning of the close icon should improve usability.
To ensure these changes don't introduce any unintended side effects:
- Conduct thorough testing of the date picker component in various scenarios and screen sizes.
- Verify that the increased width doesn't cause layout issues on smaller screens.
- Check that the close icon is properly positioned and doesn't overlap with other elements.
- Consider adding responsive design rules if necessary to maintain consistency across different device sizes.
Run the following script to check for any responsive design rules that might need updating:
#!/bin/bash # Description: Check for responsive design rules related to the date picker # Test: Search for media queries or responsive classes used with the date picker rg --type vue --type css --type html '(@media|tiny-col-|tiny-row-|tiny-span-).*tiny-(date-picker|date-editor|range)' -g '!label-inside-composition-api.vue'
23-24: LGTM! Verify visual appearance.The adjustment of the close icon's position is likely necessary due to the increased width of the date range picker.
Please verify that this positioning doesn't cause any visual issues or overlap with other elements. Consider running visual regression tests if available.
Additionally, run the following script to check for any other uses of the
.tiny-range__close-iconclass that might be affected:✅ Verification successful
Please run the following script to check for other uses of the
.tiny-range__close-iconclass:
Verified! No additional instances of
.tiny-range__close-iconwere found outside the reviewed file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other uses of the .tiny-range__close-icon class # Test: Search for other occurrences of the .tiny-range__close-icon class rg --type vue --type css --type html '\.tiny-range__close-icon' -g '!label-inside-composition-api.vue'Length of output: 185
Script:
#!/bin/bash # Description: Check for other uses of the .tiny-range__close-icon class # Test: Search for other occurrences of the .tiny-range__close-icon class in .vue, .css, and .html files rg '\.tiny-range__close-icon' --glob '!label-inside-composition-api.vue' -g '*.vue' -g '*.css' -g '*.html'Length of output: 308
examples/sites/demos/pc/app/date-picker/label-inside.vue (2)
29-30: LGTM! Verify layout impact.The increase in width from 280px to 360px for the date range picker input looks good. This change likely improves the user experience by providing more space for the selected date range.
Please verify that this width increase doesn't negatively impact the layout of surrounding elements or cause any overflow issues in the parent container.
31-32: LGTM! Verify visual alignment.The addition of
right: -4px;for the.tiny-range__close-iconlooks good. This adjustment likely improves the alignment of the close icon within the date range picker.Please verify the following:
- The close icon is visually well-aligned within the date range picker after this change.
- The icon doesn't overlap with other elements or get cut off at the edge of the input.
- The positioning works well across different screen sizes and browser zoom levels.
packages/design/saas/index.ts (2)
20-20: LGTM: Import statement for DatePanel is correct and consistent.The import statement for
DatePanelfollows the established pattern in the file and is correctly implemented.
20-20: Clarify the relationship between DatePanel and DateRange, and alignment with PR objectives.The PR objectives mention adapting the date-picker to the SMB theme, but the changes only introduce a new
DatePanelcomponent without modifying existing components. Could you please clarify:
- How does
DatePanelrelate to the existingDateRangecomponent?- How does this change align with the objective of adapting to the SMB theme?
- Are there any theme-specific modifications that should be applied to
DatePanelor other components?This information will help ensure that the PR fully meets its objectives and that the new component is properly integrated into the existing design system.
Also applies to: 44-44
examples/sites/demos/pc/app/date-picker/default-value-composition-api.vue (1)
37-37: LGTM! Verify layout impact of increased width.The increase in width for the datetime range picker from 280px to 360px aligns with the PR objective of adapting the date-picker to the SMB theme. This change likely improves the visibility and usability of the component.
Please verify that this width increase doesn't negatively impact the layout of the parent container or surrounding elements in various screen sizes. Consider testing the component in different viewport widths to ensure responsiveness.
examples/sites/demos/pc/app/date-picker/default-value.vue (1)
45-45: LGTM! Consider verifying the visual impact.The increase in width for the date-time range picker input from 280px to 360px aligns with the PR objective of adapting to the SMB theme. This change should provide better visibility and potentially accommodate longer date ranges.
To ensure the change meets the design requirements:
- Please verify that the new width of 360px matches the SMB theme specifications.
- Test the component with various date ranges to confirm that the increased width accommodates all use cases without causing layout issues.
- Consider adding before/after screenshots to the PR description to help reviewers visualize the impact of this change.
examples/sites/demos/pc/app/time-picker/size-composition-api.vue (1)
6-6: Improved order of size optionsThe reordering of the radio buttons for size selection is a good improvement. The new order (medium, default, small, mini) presents a more logical and intuitive sequence of sizes, which should enhance the user experience.
examples/sites/demos/pc/app/time-picker/size.spec.ts (2)
10-11: LGTM: Consistent height for default size time pickersThe change to 32px for both single and range time pickers in the default size improves consistency in the UI. This aligns well with the PR objective of adapting to the SMB theme.
10-23: Verify visual appearance and update documentationThe changes standardize heights across different sizes of time pickers, which improves UI consistency and aligns with the SMB theme adaptation. However, it's important to:
- Verify the visual appearance of the components with these new heights to ensure they meet design standards.
- Update any relevant documentation or design specifications to reflect these new standardized heights.
To assist with visual verification, you can run the following command to capture screenshots of the time picker components in different sizes:
Consider adding visual regression tests to automatically catch any unintended visual changes in the future.
examples/sites/demos/pc/app/date-picker/disabled.vue (1)
63-63: LGTM with suggestions: Width increase for daterange pickerThe width increase from 280px to 360px for the daterange picker aligns with the PR objective of adapting to the SMB theme. This change likely provides more space for displaying two dates, improving usability.
However, I have a few suggestions:
Consider adding a comment explaining why this specific width (360px) was chosen. This will help future maintainers understand the reasoning behind this change.
Be aware that the parent container's width is still set to 280px. This might cause layout issues or unexpected behavior. Please verify that this doesn't create any visual problems.
It's important to visually verify this change to ensure it looks correct and doesn't negatively impact the overall layout.
To ensure this change doesn't introduce any layout issues, please run the following verification steps:
- Build and run the demo application.
- Navigate to the date-picker demo page.
- Locate the daterange picker and verify that:
- The input field displays correctly without any overflow or truncation.
- The layout of surrounding elements is not negatively affected.
- The date picker dropdown aligns correctly with the input field.
If any issues are found, please adjust the width accordingly or consider updating the parent container's width as well.
examples/sites/demos/pc/app/date-picker/size-composition-api.vue (2)
6-6: Please clarify the reasoning behind the radio button order change.The order of size options has been changed, moving "small" after "default". This new order (medium, default, small, mini) doesn't follow a clear logical progression. Could you please explain the rationale behind this change? It might be helpful to consider maintaining a consistent order (e.g., from largest to smallest) for better user experience and intuition.
58-58: Approve width increase, but verify layout impact.The increase in width for the date range picker from 280px to 360px is likely to improve visibility and usability. This change aligns with the PR objective of adapting to the SMB theme.
Please ensure that this width increase doesn't negatively impact the overall layout or responsiveness of the component, especially on smaller screens. Consider adding a comment explaining the reason for this specific width, if it's related to design requirements or user feedback.
examples/sites/demos/pc/app/date-picker/size.vue (2)
6-6: Improved order of size optionsThe reordering of the radio button options for sizes is a good change. It now follows a more logical sequence from larger to smaller sizes (medium, default, small, mini), which improves user experience and aligns with common UI practices.
64-64: Increased width for date range pickerThe width increase for the date range picker from 280px to 360px could improve the readability and usability of the component. However, I have a couple of questions:
- Is this change part of the adaptation to the SMB theme mentioned in the PR objectives?
- Have you verified that this width increase doesn't negatively impact the layout in various use cases?
To ensure consistency, let's check if there are any other width definitions for date pickers in the codebase:
✅ Verification successful
The previous shell script encountered an issue with the unrecognized
vuefile type. Let's adjust the script to search for.vue,.less, and.cssfiles by specifying their extensions.
Width Adjustment for Date Range Picker Verified
The width increase from 280px to 360px has been consistently applied across all relevant components, aligning with the SMB theme adaptation objectives. This change enhances the readability and usability of the date pickers without negatively impacting the overall layout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for width definitions related to date pickers rg --type=vue --type=less --type=css 'tiny-date-editor|tiny-input__inner' -C 2 | rg 'width'Length of output: 119
Script:
#!/bin/bash # Search for width definitions related to date pickers in .vue, .less, and .css files rg 'tiny-date-editor|tiny-input__inner' -g '*.vue' -g '*.less' -g '*.css' -C 2 | rg 'width'Length of output: 1704
packages/theme-saas/src/picker/index.less (2)
25-28: LGTM: New icon container class improves layout.The addition of
.@{input-prefix-cls}__icon-containerwith flexbox alignment is a good improvement. It provides better control over the icon positioning within the input, which aligns well with the PR's objective of adapting to the SMB theme.
Line range hint
1-285: Summary: Good progress on SMB theme adaptation, with room for improvementThe changes in this file generally align well with the PR objective of adapting the date-picker to the SMB theme. The new icon container class and the use of utility classes for the close icon improve layout consistency and maintainability.
However, there are a few areas that could use attention:
- Consider adopting a more consistent approach to styling throughout the file, preferring utility classes where possible.
- Clarify the intentionality and impact of removing the
.baseCleariconclass.- Ensure that all changes maintain the existing functionality while improving the theme adaptation.
Once these points are addressed, the changes will contribute significantly to a more cohesive and maintainable SMB theme implementation for the date-picker component.
packages/theme/src/picker/index.less (2)
85-89: LGTM: Icon styling adjustmentsThe changes to the
.@{range-prefix-cls}__iconclass look good. The width adjustment and the addition of a negative margin-top will help in achieving better alignment and sizing for the SMB theme.
Line range hint
1-150: Overall feedback on date picker styling changesThe changes in this file generally align well with the PR objective of adapting the date-picker to the SMB theme. The modifications improve the visual styling and layout of various components within the date picker.
However, there are several opportunities to enhance the code's maintainability and flexibility:
Consider replacing hardcoded color values with theme variables throughout the file. This will make it easier to maintain a consistent color scheme and adapt to different themes in the future.
Look for opportunities to create mixins or utility classes for commonly repeated styles, such as the disabled state styling.
Use flexible units (like percentages or rem) or CSS variables for dimensions where possible, instead of fixed pixel values. This will improve the component's adaptability to different screen sizes and user preferences.
Ensure that all new CSS variables are properly documented in the theme variables file.
These improvements will make the date picker component more robust and easier to maintain in the long run, while still achieving the desired SMB theme adaptation.
packages/vue/src/picker/src/pc.vue (3)
122-128: Improved structure for close icon rendering.The new structure for rendering the close icon improves the component's flexibility and aligns with the PR's objective of adapting to the SMB theme. The conditional rendering based on
state.haveTriggerand the use of a separatetiny-range__close-boxclass provide better control over the icon's appearance and behavior.
130-134: Consider accessibility implications of conditional icon rendering.The trigger icon is now conditionally rendered based on
!state.isDisplayOnly. While this may improve the visual design, it's important to ensure that this doesn't negatively impact accessibility, especially for keyboard users who might rely on this icon for interaction.Please verify that:
- The date picker remains fully functional and accessible when the trigger icon is not displayed.
- There are alternative ways for users to interact with the date picker when in display-only mode.
If these conditions are met, the changes can be approved.
120-134: Overall assessment of icon rendering changes.The modifications to the icon rendering logic in this component appear to enhance its structure and flexibility, aligning well with the PR's objective of adapting to the SMB theme. The changes introduce better conditional rendering and class management for both the close and trigger icons.
While the changes generally look good, please address the following points:
- Remove the duplicate
tiny-input__iconclass.- Verify the accessibility implications of the conditional trigger icon rendering.
Once these points are addressed, the changes can be fully approved.
packages/renderless/src/date-panel/vue.ts (2)
136-136: Verify the necessity ofisShowFootercomputed property.The addition of
isShowFootercomputed property seems unrelated to the PR objective of adapting to the SMB theme. Can you clarify the purpose of this change and confirm if it's intended to be part of this PR?
137-138: LGTM! Good implementation ofbuttonTypeandbuttonSize.The addition of
buttonTypeandbuttonSizeproperties using thedesignConfigparameter is well-implemented:
- It aligns with the PR objective of adapting to the SMB theme.
- The use of optional chaining (
?.) is a good practice to handle potential undefined values.- Default values are provided as a fallback, which is a robust approach.
packages/vue/src/date-panel/src/pc.vue (1)
Line range hint
212-233: Summary: Successful adaptation to SMB theme with improved flexibility.The changes made to the date picker component successfully adapt it to the SMB theme by introducing more flexibility in the footer layout and button customization. These modifications allow the component to adjust its appearance based on different selection modes, improving its overall usability and adaptability.
Key improvements:
- Dynamic footer layout based on selection mode.
- Conditional visibility for the "Now" button.
- Customizable button size and type through dynamic bindings.
These changes enhance the component's versatility without introducing breaking changes, aligning well with the PR objectives.
To further improve the component, consider:
- Extracting complex conditions into computed properties for better readability.
- Ensuring consistent visibility rules for both footer buttons.
- Adding unit tests to verify the new dynamic behavior (if not already covered).
Great work on enhancing the date picker component!
examples/sites/demos/pc/app/date-picker/label-inside-composition-api.vue
Show resolved
Hide resolved
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
DatePanelcomponent to enhance date selection functionality.Improvements
range-separatorattributes and fixed widths for a cleaner user interface.Bug Fixes