feat(input): [input,date-picker] adapt input date-picker to the old theme style #2524
Conversation
WalkthroughThe pull request introduces several updates to the API documentation and styles of input components across multiple files. Key changes include corrections to property descriptions and types in the Changes
Possibly related PRs
Suggested labels
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 (
|
zzcr
changed the title
github-actions
bot
added
enhancement
![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 (6)
packages/theme/src/transition/icon.less (1)
34-42: Consider using CSS custom properties for animation values.For better maintainability and consistency, consider extracting the animation duration and scale values into CSS custom properties.
@keyframes bounce-in { 0% { - transform: scale(0); + transform: scale(var(--ti-icon-bounce-scale-start, 0)); } 50% { - transform: scale(1.2); + transform: scale(var(--ti-icon-bounce-scale-peak, 1.2)); } 100% { - transform: scale(1); + transform: scale(var(--ti-icon-bounce-scale-end, 1)); } }packages/theme/src/picker/index.less (1)
Line range hint
246-246: Consider addressing the TODO comment about scrollbar evaluation.The comment suggests that the scrollbar styles might not be in use. Consider evaluating and removing unused code to improve maintainability.
Would you like me to help evaluate the scrollbar usage in the codebase or create an issue to track this cleanup task?
packages/vue/src/picker/src/pc.vue (2)
Line range hint
1-3: Address technical debt: Remove tiny-date-container class.There's a TODO comment indicating that the
tiny-date-containerclass should be removed in the future. This should be tracked and addressed to prevent technical debt accumulation.Would you like me to create a GitHub issue to track the removal of this class?
Line range hint
1-157: Consider component decomposition for better maintainability.The component is quite large and handles multiple responsibilities (date picker, range picker, filter box). Consider breaking it down into smaller, more focused components:
- DatePickerInput
- DateRangeInput
- FilterBoxInput
This would improve:
- Code maintainability
- Testing
- Reusability
- Code readability
packages/theme/src/input/index.less (2)
207-214: Consider renaming the hover state color variableThe variable
--tv-Input-icon-active-border-coloris used for icon fill color on hover, but its name suggests it's for borders. Consider renaming it to something more appropriate like--tv-Input-icon-hover-colorfor better clarity.
240-240: Consider using CSS variables for padding valuesThe fixed padding values (56px, 8px) should be defined as CSS variables for better theme customization and maintenance.
- padding-right: 56px; - padding-left: 8px; + padding-right: var(--tv-Input-word-limit-padding-right, 56px); + padding-left: var(--tv-Input-word-limit-padding-left, 8px);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
examples/sites/demos/apis/input.js(2 hunks)packages/theme/src/base/index.less(1 hunks)packages/theme/src/base/old-theme.less(1 hunks)packages/theme/src/input/index.less(5 hunks)packages/theme/src/input/vars.less(1 hunks)packages/theme/src/picker/index.less(2 hunks)packages/theme/src/transition/icon.less(1 hunks)packages/vue/src/picker/src/pc.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/base/index.less
🔇 Additional comments (12)
packages/theme/src/transition/icon.less (1)
34-34: Verify the removal of vertical translation.
The changes simplify the bounce-in animation to use only scale transformations, removing the vertical movement component. While this creates a cleaner animation that might better match the old theme style, please confirm if removing translateY(-50%) was intentional.
Also applies to: 38-38, 42-42
✅ Verification successful
Based on the search results, I can now generate the final response:
Removal of vertical translation is consistent with theme styles
The removal of translateY(-50%) from the icon bounce-in animation is correct and aligns with the codebase patterns. While translateY is commonly used for element positioning (centering, etc.), it's rarely used in animation keyframes except in specific components like drawer and modal transitions. The icon animations in the theme-saas package are the only other instance of translateY in animation keyframes, but they serve a different purpose.
The simplified scale-only animation better matches the old theme style objective and creates a cleaner, more focused bounce effect.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other icon animations in the codebase use vertical translations
# to ensure consistency with the old theme style
# Search for translateY in animation keyframes
rg -B 2 -A 2 'translateY.*%' --type less
Length of output: 25971
packages/theme/src/input/vars.less (2)
Line range hint 1-77: Verify the impact of removed suffix positioning variables.
The AI summary indicates that several suffix positioning variables (--tv-Input-suffix-top-*) were removed. We should verify that this doesn't break the layout of input components with suffix elements.
Let's check for any potential layout issues:
#!/bin/bash
# Description: Verify the impact of removed suffix positioning variables
# Search for any remaining references to the removed variables
rg --type=less "tv-Input-suffix-top"
# Search for suffix-related CSS classes to understand the new positioning approach
rg --type=less "\.tiny-input__suffix"57-58: LGTM! Icon size variable change improves semantic correctness.
The change from --tv-font-size-l to --tv-icon-size better reflects the variable's purpose and promotes consistency in icon sizing across components.
Let's verify that this change doesn't cause any icon size regressions:
✅ Verification successful
Let's analyze the usage patterns of these variables to verify the impact of the change.
Based on the search results, I can now generate the final response:
Icon size change is consistent with design system
The change from --tv-font-size-l to --tv-icon-size aligns with the design system where:
--tv-icon-sizeis the standard variable used across components for icon sizing (16px)- Many other components like Select, Dropdown, Alert, etc. already use this variable
- This change promotes consistency in icon sizing across the UI
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other components that might be affected by this icon size change
# Search for any other usage of the old font size variable for icons
rg --type=less "font-size-l.*icon"
# Search for other components using this input icon size variable
rg --type=less "tv-Input-icon-font-size"
Length of output: 238
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Search for any usage of font-size-l in less files
rg --type=less "font-size-l"
# 2. Search for icon-related size variables
rg --type=less "icon.*size|size.*icon"
# 3. Search specifically for Input-icon references
rg --type=less "Input.*icon"
# 4. Look for the icon-size variable usage
rg --type=less "tv-icon-size"
Length of output: 42334
packages/theme/src/picker/index.less (2)
116-122: LGTM! Improved icon interaction handling.
The changes enhance the UI interaction by:
- Adding proper cursor feedback for the close icon
- Preventing icon overlap through proper visibility rules
227-227: LGTM! CSS syntax optimization.
Good optimization by removing unnecessary 'px' unit for zero value, following CSS best practices.
packages/theme/src/input/index.less (4)
151-152: LGTM: Proper vertical alignment for prefix/suffix elements
The positioning ensures consistent vertical alignment of prefix/suffix elements within the input.
271-271: LGTM: Well-implemented display-only mode
The display-only mode implementation properly handles:
- Text overflow with ellipsis
- Vertical alignment
- Mask support for formatted display
- Proper icon sizing
Line range hint 320-326: LGTM: Clean implementation of underline mode
The underline mode implementation properly removes all borders except the bottom one while maintaining the input's core styling.
257-257: Verify if small and mini variants should share the same font-size
Both size variants use var(--tv-Input-small-font-size). Consider if the mini variant should have its own smaller font size variable.
Also applies to: 265-265
✅ Verification successful
Small and mini variants intentionally share the same font size
Looking at the variable definitions in vars.less, there's a specific comment // small和mini尺寸字体大小 (meaning "small and mini size font size") followed by the shared variable --tv-Input-small-font-size. This indicates that using the same font size for both variants is an intentional design decision.
The differentiation between small and mini variants appears to be primarily through other properties like height and padding, rather than font size.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's a separate mini font size variable defined
rg --type less "tv-Input-mini-font-size"
Length of output: 40
Script:
#!/bin/bash
# Let's check the variable definitions and usage patterns for Input size variants
rg --type less "tv-Input.*font-size" -B 2 -A 2
# Also check the actual CSS rules for small and mini variants
rg --type less "\.tv-input--(?:small|mini)" -B 2 -A 5
Length of output: 3316
examples/sites/demos/apis/input.js (2)
132-132: LGTM! Fixed typo in display-only-content description.
The description now correctly refers to "modelValue" instead of "modelVale".
419-419:
Inconsistency in size property documentation between Chinese and English descriptions.
The Chinese description states the size property is valid when type="textarea", while the English description states it's valid when type!="textarea". This contradiction needs to be resolved to prevent confusion.
Let's verify the correct behavior:
packages/theme/src/base/old-theme.less (1)
414-416: LGTM! Height adjustments align with old theme adaptation.
The changes to height variables look good and maintain a consistent sizing scale:
- Reduced lg height from 40px to 36px
- Reduced xl height from 48px to 42px
- Explicitly set default height to sm (28px)
Let's verify the components affected by these height changes:
packages/vue/src/picker/src/pc.vue
Outdated
| <i class="tiny-input__icon tiny-input__suffix"> | ||
| <transition name="tiny-transition-icon-scale-in"> | ||
| <div v-if="state.haveTrigger"> | ||
| <component | ||
| :is="state.showClose ? clearIcon : null" | ||
| @click="handleClickIcon" | ||
| class="tiny-range__close-icon" | ||
| /> | ||
| </div> | ||
| <component | ||
| v-if="state.haveTrigger" | ||
| :is="state.showClose ? clearIcon : null" | ||
| @click="handleClickIcon" | ||
| class="tiny-range__close-icon" | ||
| /> | ||
| </transition> | ||
| <component | ||
| v-if="!state.isDisplayOnly" | ||
| :is="state.triggerClass" | ||
| class="tiny-input__icon tiny-range__icon tiny-input__suffix" | ||
| /> | ||
| <component v-if="!state.isDisplayOnly" :is="state.triggerClass" class="tiny-range__icon" /> |
There was a problem hiding this comment.
💡 Codebase verification
CSS classes need to be added to theme files
The verification revealed that while the theme files contain various tiny-input and transition-related styles, the specific classes used in the component are missing:
tiny-transition-icon-scale-intransition is not defined (though there is ascale-inkeyframe animation)tiny-range__close-iconandtiny-range__iconclasses are not found in the theme files
Required additions to theme files:
- Add the
tiny-transition-icon-scale-intransition class that utilizes the existingscale-inkeyframe - Add the range-specific icon classes:
tiny-range__close-iconandtiny-range__icon
🔗 Analysis chain
LGTM! Verify CSS classes and transitions.
The icon structure changes look good and align with the old theme style. The class naming follows BEM conventions consistently.
Let's verify the existence of the CSS classes and transitions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CSS classes and transitions exist in the codebase
# Check for the transition class
echo "Checking for transition class..."
rg "tiny-transition-icon-scale-in" --type less
# Check for the range icon classes
echo "Checking for range icon classes..."
rg "tiny-range__close-icon|tiny-range__icon" --type less
Length of output: 259
Script:
#!/bin/bash
# Let's try a broader search for CSS classes and transitions
# Search for any files containing 'transition' related styles
echo "Checking for transition-related styles..."
rg -A 3 "\.tiny-transition|@keyframes.*scale-in" --type css --type less --type scss
# Search for range-related styles with broader context
echo "Checking for range-related styles..."
rg -A 3 "\.tiny-range|\.tiny-input" --type css --type less --type scss
# Let's also check Vue files for similar class usage
echo "Checking for similar class usage in Vue files..."
rg "tiny-(range|input)__(icon|suffix|close)" --type vue
Length of output: 585
Script:
#!/bin/bash
# Let's try without file type restrictions since the types aren't recognized
# Search for transition-related styles
echo "Checking for transition-related styles..."
fd -e css -e less . | xargs rg -A 3 "\.tiny-transition|@keyframes.*scale-in" || true
# Search for range and input related styles
echo "Checking for range and input related styles..."
fd -e css -e less . | xargs rg -A 3 "\.tiny-range|\.tiny-input" || true
# Check for class usage in Vue files
echo "Checking for class usage in components..."
fd -e vue . | xargs rg "tiny-(range|input)__(icon|suffix|close)" || true
# Also check theme directories specifically
echo "Checking theme directories..."
fd . "packages/theme" | xargs rg "tiny-(range|input|transition)" || true
Length of output: 9155
zzcr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/theme-saas/src/picker/index.less(3 hunks)packages/theme/src/picker/index.less(2 hunks)packages/vue/src/picker/src/pc.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/theme/src/picker/index.less
- packages/vue/src/picker/src/pc.vue
🔇 Additional comments (1)
packages/theme-saas/src/picker/index.less (1)
Line range hint 25-29: LGTM! Good use of flexbox for icon alignment.
The new icon container class follows BEM naming conventions and uses flexbox for proper alignment, which is a modern and maintainable approach.
| width: 25px; | ||
| float: right; | ||
| @apply leading-8; | ||
| @apply mr-5; | ||
| @apply flex; | ||
| @apply justify-center; | ||
| @apply items-center; | ||
| @apply h-5; | ||
| @apply mr-6; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing float when using flexbox.
The current implementation mixes flexbox (display: flex, justify-center, items-center) with float positioning (float: right). This combination can lead to unpredictable layout behavior. Consider:
- Removing the
floatproperty since flexbox provides better control - Using relative units (em/rem) instead of fixed pixel width for better responsiveness
.@{range-prefix-cls}__close-icon {
@apply absolute;
@apply ~'right-1.5';
@apply text-base;
- width: 25px;
- float: right;
+ width: 1.5625rem; /* 25px converted to rem */
@apply leading-8;
@apply mr-5;
@apply flex;
@apply justify-center;
@apply items-center;
}📝 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.
| width: 25px; | |
| float: right; | |
| @apply leading-8; | |
| @apply mr-5; | |
| @apply flex; | |
| @apply justify-center; | |
| @apply items-center; | |
| @apply h-5; | |
| @apply mr-6; | |
| } | |
| width: 1.5625rem; /* 25px converted to rem */ | |
| @apply leading-8; | |
| @apply mr-5; | |
| @apply flex; | |
| @apply justify-center; | |
| @apply items-center; | |
| } |
| .baseClearicon { | ||
| @apply mr-2; | ||
| @apply !transform-none; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
The !transform-none override appears unnecessary
The codebase search shows that there's no default transform being applied to .baseClearicon that would need to be overridden. The only transform found in the picker components is for positioning text elements (transform: translateY(-50%)), which is unrelated to the clear icon. The !transform-none declaration can be safely removed as it's overriding a non-existent transform.
🔗 Analysis chain
Verify the necessity of transform override.
While the margin adjustment improves spacing, the use of !transform-none suggests overriding a previous transform. Please verify if this override is necessary or if the original transform can be removed from the source.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any transform properties applied to .baseClearicon
rg -l "transform" --type less
rg "\.baseClearicon.*transform" --type less
Length of output: 6152
Script:
#!/bin/bash
# Let's check the picker/index.less files in both theme packages for any transform styles
cat packages/theme-saas/src/picker/index.less packages/theme/src/picker/index.less | rg -A 2 -B 2 "transform|\.baseClearicon"
Length of output: 535
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
sizeproperty for thetextareatype.Style
Chores