fix(time-picker): [time-picker] fixed a button style issue in the Sass theme #2330
Conversation
WalkthroughThe changes in this pull request involve the addition of new styles to the time range picker component in 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 (5)
packages/theme-saas/src/time-range/index.less (3)
38-46: LGTM! Consider adding responsive padding.The footer styles look good and are consistent with the project's styling approach. The flexbox layout is appropriate for positioning footer elements.
Consider adding responsive padding for better mobile experience:
- @apply py-3 px-4; + @apply py-3 px-4 sm:px-6;
48-60: LGTM! Consider adding focus styles for accessibility.The button styles provide a good base for appearance and are consistent with the project's styling approach.
Consider adding focus styles for better accessibility:
@apply outline-0; + @apply focus:ring-2 focus:ring-color-brand focus:ring-offset-2;
61-75: Enhance cancel button styles for consistency and accessibility.While the differentiation between cancel and confirm buttons is good, the cancel button lacks hover and focus styles.
Consider adding hover and focus styles to the cancel button for consistency and improved accessibility:
&.cancel { @apply text-color-brand; @apply text-left; @apply rounded; + @apply hover:bg-color-brand hover:bg-opacity-10; + @apply focus:ring-2 focus:ring-color-brand focus:ring-offset-2; } &.confirm { @apply text-color-text-inverse; @apply bg-color-brand; @apply rounded; + @apply focus:ring-2 focus:ring-color-brand focus:ring-offset-2; &:hover { @apply bg-color-brand-hover; } }packages/vue/src/time/src/pc.vue (1)
39-41: LGTM! Consider a minor improvement for clarity.The addition of the 'cancel' class to the button aligns with the PR objective of fixing a button style issue. This change is consistent with the new styles introduced in the
index.lessfile.For improved readability, consider using a computed property for the button's class. This would make the template cleaner and move the logic to the script section. For example:
<template> <tiny-button v-if="!state.showTimePickerButton" :class="cancelButtonClass" @click="handleCancel()"> {{ t('ui.datepicker.cancel') }} </tiny-button> </template> <script> export default { // ... other code ... computed: { cancelButtonClass() { return ['tiny-time-panel__btn', 'cancel'] } } } </script>This approach would make it easier to add or modify classes in the future without cluttering the template.
packages/vue/src/time-range/src/pc.vue (1)
81-81: LGTM! Consider applying the same class structure to the confirm button.The addition of the
cancelclass to the cancel button is a good improvement for more specific styling. This change follows the BEM (Block Element Modifier) methodology, which is a best practice for CSS naming conventions.For consistency, consider applying a similar class structure to the confirm button. You could add a
confirmclass to it as well. For example:- :class="{ - 'tiny-time-range-picker__btn': !state.showTimePickerRangeButton, - confirm: !state.disabled && !state.showTimePickerRangeButton - }" + :class="[ + 'tiny-time-range-picker__btn', + { confirm: !state.disabled && !state.showTimePickerRangeButton } + ]"This change would make the class structure consistent between the cancel and confirm buttons, improving the overall maintainability of the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/theme-saas/src/time-range/index.less (1 hunks)
- packages/vue/src/time-range/src/pc.vue (1 hunks)
- packages/vue/src/time/src/pc.vue (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/theme-saas/src/time-range/index.less (1)
37-76: Summary: Styling changes align with PR objectives and enhance component usability.The introduced styles for the footer and buttons in the time range picker component successfully address the PR's objective of fixing a button style issue. The changes improve the component's appearance and functionality while maintaining consistency with the project's styling approach.
The suggestions made for accessibility and responsiveness should further enhance the user experience without significantly altering the intended design. These improvements align well with best practices in web development.
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