refactor(numeric): [numeric]refactor numeric theme vars #2200
Conversation
WalkthroughThe pull request introduces multiple modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
![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 (6)
examples/sites/demos/pc/app/numeric/dynamic-disabled-composition-api.vue (1)
5-7: LGTM! Consider adding comments for clarity.The additional
<tiny-numeric>components effectively demonstrate various configurations, enhancing the example. This aligns well with the PR's refactoring objective.Consider adding comments above each new
<tiny-numeric>component to describe its specific configuration. This would improve readability and make the purpose of each instance clearer. For example:<!-- Numeric input with 'kg' unit --> <tiny-numeric v-model="value1" :disabled="disabled" unit="kg" class="mr20"></tiny-numeric> <!-- Numeric input without controls and left-side display --> <tiny-numeric v-model="value1" :disabled="disabled" :controls="false" show-left class="mr20"></tiny-numeric> <!-- Numeric input with controls on the right --> <tiny-numeric v-model="value1" :disabled="disabled" controls-position="right" class="mr20"></tiny-numeric>examples/sites/demos/pc/app/numeric/numeric-size.spec.ts (1)
Line range hint
22-27: LGTM, but consider using more robust selectors.The change from
.nth(2)to.nth(3)for the mini numeric component is consistent with the previous change for the small numeric component.To make the tests more robust and less dependent on the exact order of elements, consider using more specific selectors. For example:
const miniNumeric = page.locator('.tiny-numeric.tiny-numeric--mini')This approach would be more resilient to changes in the DOM structure and make the tests easier to maintain.
packages/theme/src/numeric/index.less (4)
63-63: Translate Comments to English for ConsistencyThe comment is written in Chinese:
// 保证文本居中. To maintain consistency and readability for all developers, please translate comments to English.Update the comment as follows:
line-height: 1.5; -// 保证文本居中 +// Ensure the text is vertically centered
84-84: Avoid Duplicate Border PropertyThe
borderproperty is redefined multiple times without overriding other styles. This may be redundant.Consider reviewing and consolidating the
borderproperty to ensure it's only defined when necessary.
89-89: Translate Comments to English for ConsistencyThe comment is written in Chinese:
// 保证文本居中. Please translate it to English.Update the comment as follows:
&:focus { outline: 0; - .placeholder(@color: var(--tv-Numeric-input-placeholder-text-color)); - // 保证文本居中 + .placeholder(@color: var(--tv-Numeric-input-placeholder-text-color)); + // Ensure the text is vertically centered }
323-323: Translate Comments to English for ConsistencyThe comment
// 消除父子元素间多出来的空白is in Chinese. Please translate the comment for consistency and readability.Update the comment as follows:
font-size: 0; -// 消除父子元素间多出来的空白 +// Eliminate extra whitespace between parent and child elements
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- examples/sites/demos/pc/app/numeric/dynamic-disabled-composition-api.vue (2 hunks)
- examples/sites/demos/pc/app/numeric/dynamic-disabled.vue (2 hunks)
- examples/sites/demos/pc/app/numeric/numeric-size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/numeric/numeric-size.spec.ts (1 hunks)
- examples/sites/demos/pc/app/numeric/numeric-size.vue (1 hunks)
- packages/theme/src/numeric/index.less (8 hunks)
- packages/theme/src/numeric/vars.less (1 hunks)
🔇 Additional comments (9)
examples/sites/demos/pc/app/numeric/numeric-size-composition-api.vue (2)
Line range hint
1-20: Clarification needed on PR objectives and changesThe changes enhance the demo by showcasing all available sizes of the
<tiny-numeric>component, including the default size. However, there are a few points that need clarification:
The AI summary mentioned the removal of a
<tiny-numeric>component after the mini-sized one, but this removal is not visible in the provided diff. Could you confirm if this removal was intended and if it's missing from the diff?The PR objective states "refactor numeric theme vars", but the changes in this file don't directly reflect this. Could you elaborate on how these changes relate to the refactoring effort?
Consider updating the PR description to accurately reflect the changes made in this file, specifically mentioning the addition of the default-sized component to the demo.
To check for any related changes that might not be visible in this file:
#!/bin/bash # Description: Check for related changes in the PR # List all changed files in the PR echo "Changed files in this PR:" git diff --name-only HEAD~1 # Check for any changes related to numeric theme variables echo "Changes related to numeric theme variables:" git diff HEAD~1 | grep -i 'numeric.*theme\|theme.*numeric'This will help us understand the full scope of changes related to the numeric component and theme variables.
5-6: LGTM, but clarification needed on PR objectivesThe addition of a default-sized
<tiny-numeric>component enhances the demo by showcasing all available sizes. However, this change doesn't seem to directly relate to the stated PR objective of "refactor numeric theme vars". Could you clarify how this change aligns with the refactoring effort?To improve code readability, consider adding a comment explaining the purpose of this default-sized component:
+ <!-- Default size --> <tiny-numeric v-model="value"></tiny-numeric> <br /><br />To verify the impact of this change, let's check for any related changes in the theme variables:
✅ Verification successful
Verified: Numeric theme variables refactored as per PR objectives
The recent changes successfully refactor the numeric theme variables from
--ti-to--tv-, aligning with the PR's goal. Additionally, the addition of the<tiny-numeric>component enhances the demo by showcasing the updated theme variations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for changes in numeric theme variables # Search for files that might contain numeric theme variables echo "Files potentially containing numeric theme variables:" fd -e scss -e css -e less | grep -i 'theme\|var\|numeric' # Search for any recent changes related to numeric variables echo "Recent changes related to numeric variables:" git log -p -S "numeric" --since="1 week ago" -- '*.scss' '*.css' '*.less'Length of output: 1081
Script:
#!/bin/bash # Description: Check for changes in numeric theme variables # Search for files that might contain numeric theme variables echo "Files potentially containing numeric theme variables:" fd --extensions scss,css,less | grep -iE 'theme|var|numeric' # Search for any recent changes related to numeric variables echo "Recent changes related to numeric theme variables in the last week:" git log -p -S "numeric" --since="1 week ago" -- '*.scss' '*.css' '*.less'Length of output: 36514
examples/sites/demos/pc/app/numeric/numeric-size.vue (1)
5-6: LGTM! Consider documentation updates.The addition of a
<tiny-numeric>instance without a size attribute is a good way to demonstrate the default size. This change aligns well with the PR objective of refactoring numeric theme variables.As this is a demo file, please verify if any corresponding documentation needs to be updated to reflect these changes in the size demonstrations.
Also, could you clarify if an instance of
<tiny-numeric>without a size attribute was removed as mentioned in the AI summary? It's not visible in the provided diff, and this information might be relevant for a complete understanding of the changes.examples/sites/demos/pc/app/numeric/dynamic-disabled-composition-api.vue (2)
23-28: LGTM! Scoped style addition is appropriate.The addition of the scoped style for
.mr20is a good practice. It provides consistent spacing between the numeric input components and ensures that the styling is limited to this component.
Line range hint
1-28: Overall, the changes effectively demonstrate the<tiny-numeric>component's capabilities.The additions to this file align well with the PR's objective of refactoring numeric theme variables. The new examples showcase various configurations, providing a more comprehensive demonstration of the component's features. The consistent use of styling and bindings maintains code quality and readability.
examples/sites/demos/pc/app/numeric/dynamic-disabled.vue (1)
32-36: Good addition of scoped style for consistent spacing.The introduction of a scoped style block with the
.mr20class is a good practice. It ensures consistent spacing between components and improves the overall layout of the demo.The use of scoped styles in Vue components is recommended for better encapsulation and maintainability.
examples/sites/demos/pc/app/numeric/numeric-size.spec.ts (1)
15-20: LGTM, but please verify the DOM structure.The change from
.nth(1)to.nth(2)for the small numeric component suggests that the order of elements has changed. This is consistent with the large numeric still being the first element.To ensure this change is correct, please verify the new DOM structure. You can run the following script to check the structure of
.tiny-numericelements:This will show the first three
.tiny-numericelements. Verify that the second one (index 1) has thetiny-numeric--smallclass.packages/theme/src/numeric/index.less (1)
76-76: Variable Name Typo: Correct to Maintain ConsistencyThe variable
--tv-Numeric-input-placeholder-text-colorshould be consistently named throughout the codebase. Verify that the variable name is correct and matches its definition.Please ensure that
--tv-Numeric-input-placeholder-text-coloris defined invars.lessand used consistently.packages/theme/src/numeric/vars.less (1)
13-100: Well-organized refactoring of numeric theme variablesThe refactoring systematically updates variable names to the new naming convention, enhancing consistency and maintainability across the codebase.
| <tiny-numeric v-model="value1" :disabled="disabled" unit="kg" class="mr20"></tiny-numeric> | ||
| <tiny-numeric v-model="value1" :disabled="disabled" :controls="false" show-left class="mr20"></tiny-numeric> | ||
| <tiny-numeric v-model="value1" :disabled="disabled" controls-position="right" class="mr20"></tiny-numeric> |
There was a problem hiding this comment.
Consider using separate v-model bindings for each component.
The additions effectively demonstrate various configurations of the <tiny-numeric> component, which is great for showcasing its versatility. However, using the same v-model="value1" for all components might lead to unexpected behavior, as changing one component will update all others.
Consider using separate v-model bindings for each component to ensure independent behavior. For example:
<tiny-numeric v-model="value1" :disabled="disabled" unit="kg" class="mr20"></tiny-numeric>
<tiny-numeric v-model="value2" :disabled="disabled" :controls="false" show-left class="mr20"></tiny-numeric>
<tiny-numeric v-model="value3" :disabled="disabled" controls-position="right" class="mr20"></tiny-numeric>Then update the data() section accordingly:
data() {
return {
value1: 1,
value2: 1,
value3: 1,
// ... other properties
}
}The consistent use of the "mr20" class and the variety of component configurations are good practices for maintaining uniform spacing and demonstrating component flexibility.
| @@ -19,11 +19,11 @@ | |||
| @input-prefix-cls: ~'@{css-prefix}input'; | |||
|
|
|||
| .@{numeric-prefix-cls} { | |||
| .component-css-vars-numeric(); | |||
| .inject-Numeric-vars(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Consistent Naming Convention for Mixins
The mixin name .inject-Numeric-vars(); uses PascalCase. For consistency with other mixin calls in the codebase, consider using all lowercase letters.
Consider renaming the mixin to:
.inject-numeric-vars();| text-align: center; | ||
| cursor: pointer; | ||
| user-select: none; |
There was a problem hiding this comment.
Property user-select: none; May Affect Accessibility
Setting user-select: none; prevents users from selecting text, which may affect accessibility.
Consider whether preventing text selection is necessary. If not, you might want to remove this property to improve user experience.
| border-bottom: 1px solid var(--tv-Numeric-input-normal-border-color); | ||
| border-left: 1px solid var(--tv-Numeric-input-normal-border-color); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Border Properties May Need Consistent Variables
The border properties use hardcoded colors. To maintain consistency and theming capabilities, consider using CSS variables.
Update the border properties to use the appropriate CSS variables, such as --tv-Numeric-input-border-color.
&__increase {
border-radius: 0 4px 0 0;
- border-bottom: 1px solid var(--tv-Numeric-input-normal-border-color);
- border-left: 1px solid var(--tv-Numeric-input-normal-border-color);
+ border-bottom: 1px solid var(--tv-Numeric-input-border-color);
+ border-left: 1px solid var(--tv-Numeric-input-border-color);Ensure that --tv-Numeric-input-border-color is defined and represents the intended color.
📝 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.
| border-bottom: 1px solid var(--tv-Numeric-input-normal-border-color); | |
| border-left: 1px solid var(--tv-Numeric-input-normal-border-color); | |
| border-bottom: 1px solid var(--tv-Numeric-input-border-color); | |
| border-left: 1px solid var(--tv-Numeric-input-border-color); |
| --tv-Numeric-input-dividing-border-color: var(--tv-color-border-divider); | ||
| // 分割线高度 | ||
| --ti-numeric-input-dividing-border-height: var(--ti-common-space-5x, 20px); | ||
| // 禁用时分割线display(hide) | ||
| --ti-numeric-input-dividing-border-display: none; | ||
| --tv-Numeric-input-dividing-border-height: 20px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent naming of 'dividing-border' and 'divider' variables
There is inconsistency in the variable naming between 'dividing-border' and 'divider'. For example:
- Line 23~:
--tv-Numeric-input-dividing-border-color - Line 25~:
--tv-Numeric-input-dividing-border-height - Line 67~:
--tv-Numeric-input-divider-height-medium - Line 78~:
--tv-Numeric-input-divider-height-small - Line 89~:
--tv-Numeric-input-divider-height-mini
Consider standardizing the variable names to use either 'divider' or 'dividing-border' consistently for better maintainability and readability.
Apply this diff to rename the variables to use 'divider':
- --tv-Numeric-input-dividing-border-color: var(--tv-color-border-divider);
+ --tv-Numeric-input-divider-color: var(--tv-color-border-divider);
- --tv-Numeric-input-dividing-border-height: 20px;
+ --tv-Numeric-input-divider-height: 20px;Update all relevant references accordingly.
Also applies to: 67-67, 78-78, 89-89
| // medium尺寸图标大小 | ||
| --ti-numeric-input-icon-size-medium: var(--ti-common-font-size-2, 16px); | ||
| --tv-Numeric-input-icon-size-medium: var(--tv-icon-size); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize icon size variables across different sizes
In the default and medium sizes, the icon size is set using a variable (var(--tv-icon-size)), while in small and mini sizes, the icon sizes are hardcoded to 14px. For consistency, consider defining variables for the small and mini icon sizes.
Define new variables for small and mini icon sizes:
+ --tv-icon-size-sm: 14px;
+ --tv-icon-size-xs: 12px; /* Adjust if necessary */
- --tv-Numeric-input-icon-size-small: 14px;
+ --tv-Numeric-input-icon-size-small: var(--tv-icon-size-sm);
- --tv-Numeric-input-icon-size-mini: 14px;
+ --tv-Numeric-input-icon-size-mini: var(--tv-icon-size-xs);This approach enhances scalability and maintains consistency across different size variants.
Also applies to: 76-76, 87-87
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
<tiny-numeric>components with varying properties in multiple demo files, enhancing the numeric input experience.Bug Fixes
Chores