refactor(button): [button] refactor the component variables of the button component. #2193
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the button component across various files. Key modifications include the addition of a Changes
Possibly related PRs
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (7)
packages/theme/src/button/snippet.js (2)
2-29: Consider uncommenting relevant properties and removing duplicatesMost properties in the
propsarray are commented out. If these properties are intended for future use, consider adding a TODO comment explaining the plan for implementing them. Additionally, there are duplicate entries for some properties (e.g., 'font-size|字号'). Please review and remove any unintended duplications.Here's a suggested cleanup:
const props = [ 'text-color|文本色', + // TODO: Implement the following properties in future iterations // 'bg-color|背景色', // 'border-color|边框色', // 'icon-color|图标色', // ... (other commented properties) - // 'font-size|字号', // 'height|高度', // 'padding-x|水平间距', // 'min-width|最小宽度', ]
31-48: Clarify the intention behind commented suffixesThe
suffixarray has most of its items commented out, with only the '-ghost|幽灵时' (ghost state) uncommented. This selective implementation raises questions:
- Is the ghost state the only one currently needed?
- Are there plans to implement the other states in the future?
Consider adding a comment explaining the current implementation strategy and future plans for the commented-out suffixes.
Here's a suggested addition:
const suffix = [ + // TODO: Implement other button states as needed. Currently, only the ghost state is in use. // '-default|默认时', // ... (other commented suffixes) '-ghost|幽灵时' // ... (remaining commented suffixes) ]packages/vue/src/button/src/index.ts (1)
52-54: Suggestion: Update documentation and tests.The changes to the
sizeproperty align with the PR objective to reconstruct the button component's variables. While the changes themselves are relatively small, they introduce a new default behavior that should be reflected in the component's documentation and tests.Please ensure that:
- The component's documentation is updated to reflect the new default size option.
- Existing tests are updated if they rely on the previous default behavior.
- New tests are added to cover the 'default' size option.
This will help maintain the quality and consistency of the codebase.
packages/theme/src/button/index-mini.less (1)
28-28: Ensure all code comments are in English for consistency.Some comments in the code are written in Chinese. To maintain consistency and aid understanding for all contributors, it's recommended to use English for all code comments.
Also applies to: 70-70, 82-82, 91-91, 166-166, 171-171
packages/theme/src/button/index.less (2)
28-339: Standardize code comments to English for consistency.The code contains comments in Chinese, such as in lines 28, 39, 68, and others. For better collaboration across international teams, consider writing all code comments in English.
201-202: Adjustvertical-alignfor better SVG icon alignment.Using
vertical-align: text-top;on.tiny-svgmay not center the icon as intended. Consider usingvertical-align: middle;to vertically center the SVG icon within the button more effectively.packages/theme/src/button/vars.less (1)
34-36: Resolve TODOs and replace hard-coded values with variablesThe comments indicate uncertainty about replacing hard-coded values with variables (e.g.,
space-base * 6). Please decide whether to use variable expressions for these values to enhance maintainability and consistency.Do you need assistance in refactoring these values? I can help generate the necessary code changes or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- examples/sites/demos/pc/app/button/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/button/size.vue (1 hunks)
- packages/theme/src/button/index-mini.less (1 hunks)
- packages/theme/src/button/index.less (1 hunks)
- packages/theme/src/button/snippet.js (1 hunks)
- packages/theme/src/button/test-index.less (1 hunks)
- packages/theme/src/button/test.less (1 hunks)
- packages/theme/src/button/vars.less (1 hunks)
- packages/vue/src/button/src/index.ts (1 hunks)
🔇 Additional comments (20)
packages/theme/src/button/snippet.js (1)
1-1: LGTM: Appropriate prefix for button CSS variablesThe
prefixarray is well-defined with both English and Chinese representations, which is good for internationalization.packages/vue/src/button/src/index.ts (2)
54-54: LGTM: Validator function updated correctly.The validator function has been appropriately updated to include 'default' as a valid option for the
sizeproperty. This change is consistent with the new default value and maintains backwards compatibility by retaining all previous valid options.
52-52: Approve change with verification suggestion.The change from an empty string to 'default' as the default value for the
sizeproperty improves clarity. The comment suggests this won't affect existing templates, which is good for backwards compatibility.To ensure this change doesn't unexpectedly impact existing implementations, please run the following verification:
✅ Verification successful
Default size change verified and approved.
No instances of the Button component using an empty
sizeprop were found, ensuring that changing the default to'default'is safe and does not affect existing templates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any usage of Button component with empty size prop # Search for Button component usage with empty size prop rg --type vue --type typescript 'Button.*size\s*=\s*[\'"][\'"]\s*' -g '!**/node_modules/**' # Search for any programmatic setting of button size to empty string rg --type vue --type typescript 'button.*size\s*=\s*[\'"][\'"]\s*' -g '!**/node_modules/**' echo "If there are no results, it's likely safe to proceed with the change." echo "If there are results, review each case to ensure the new default won't cause issues."Length of output: 94
Script:
#!/bin/bash # Description: Check for any usage of Button component with empty size prop # Search for Button component usage with empty size prop rg --type vue --type typescript "Button.*size\s*=\s*['\"]['\"]\s*" -g '!**/node_modules/**' # Search for any programmatic setting of button size to empty string rg --type vue --type typescript "button.*size\s*=\s*['\"]['\"]\s*" -g '!**/node_modules/**' echo "If there are no results, it's likely safe to proceed with the change." echo "If there are results, review each case to ensure the new default won't cause issues."Length of output: 585
Script:
#!/bin/bash # Description: Check for any usage of Button component with empty size prop # Search for Button component usage with empty size prop in .vue and .ts files rg "Button.*size\s*=\s*['\"]['\"]\s*" -g "*.vue" -g "*.ts" -g "!**/node_modules/**" # Search for any programmatic setting of button size to empty string in .vue and .ts files rg "button.*size\s*=\s*['\"]['\"]\s*" -g "*.vue" -g "*.ts" -g "!**/node_modules/**" echo "If there are no results, it's likely safe to proceed with the change." echo "If there are results, review each case to ensure the new default won't cause issues."Length of output: 513
examples/sites/demos/pc/app/button/size-composition-api.vue (6)
8-8: LGTM: Improved button size consistencyThe addition of the small-sized button completes the range of button sizes for regular buttons. The placement before the mini-sized button follows a logical size progression, enhancing the overall consistency of the component.
17-17: LGTM: Consistent small-sized button for plain styleThe addition of the small-sized plain button maintains consistency with the regular button section. The placement and attributes are correct, ensuring a uniform approach across different button types.
26-26: LGTM: Consistent small-sized button for round styleThe addition of the small-sized round button maintains consistency with other button type sections. The placement and attributes are correct, ensuring a uniform approach across different button styles.
35-35: LGTM: Consistent small-sized button for circular style with correct icon usageThe addition of the small-sized circular button maintains consistency with other button type sections. The placement, attributes, and icon usage are correct, ensuring a uniform approach across all button styles, including those with icons.
44-44: LGTM: Consistent small-sized button for pure icon styleThe addition of the small-sized pure icon button completes the consistency across all button types. The placement, attributes, icon usage, and text type are correctly applied, ensuring a uniform approach for pure icon buttons as well.
8-44: Overall assessment: Excellent improvements to button size consistencyThe changes in this file successfully address the PR objective of reconstructing the component variables of the button component. By adding small-sized buttons consistently across all button types (regular, plain, round, circular, and pure icon), the component now offers a more complete and uniform set of size options. This enhancement improves the overall consistency and usability of the button component, making it more flexible for various design requirements.
The changes are well-implemented, following a consistent pattern in placement and attribute usage across different button styles. This consistency will likely improve the developer experience when working with the component.
examples/sites/demos/pc/app/button/size.vue (5)
8-8: LGTM: Addition of small-sized regular buttonThe addition of a small-sized button completes the range of sizes for regular buttons, improving the comprehensiveness of the demonstration.
17-17: LGTM: Addition of small-sized plain buttonThe addition of a small-sized plain button maintains consistency with the regular buttons section and completes the range of sizes for plain buttons.
26-26: LGTM: Addition of small-sized round buttonThe addition of a small-sized round button with success type maintains consistency with previous sections and completes the range of sizes for round buttons.
35-35: LGTM: Addition of small-sized circular buttonThe addition of a small-sized circular button with warning type and edit icon maintains consistency with previous sections and completes the range of sizes for circular buttons.
44-44: LGTM: Addition of small-sized pure icon button and overall assessmentThe addition of a small-sized pure icon button with text type maintains consistency with previous sections and completes the range of sizes for pure icon buttons.
Overall, these changes successfully reconstruct the button component by adding small-sized buttons across all button types and styles. This enhances the completeness of the size demonstration and improves the component's flexibility.
packages/theme/src/button/index.less (2)
40-49: Verify the usage of the%()function in Less.The
%()function used in lines 41-44 is not a standard Less function. Please ensure that this function is defined in your codebase or imported files. If it's a custom function, make sure it behaves as expected to prevent any compilation issues.
207-217: Clarify special case handling for default theme border colors.In lines 207-217, there is special handling for the default theme’s border colors that differs from other themes. Ensure this exception is well-documented and verify that it does not cause unintended styling issues.
packages/theme/src/button/vars.less (2)
12-271: Great job on organizing variables and maintaining naming consistencyThe refactored variables use a consistent naming convention with the
--tv-Button-prefix, which improves clarity and maintainability. The reorganization into categories like dimensions, colors, and states enhances the structure of the styling variables.
68-68: Verify the appropriateness of the new value for--tv-Button-min-width-miniThe comment mentions that the old value was
24px, which was insufficient compared topadding*2. Ensure that the new value of60pxaligns with the design requirements and provides adequate spacing.Run the following script to search for usages of
--tv-Button-min-width-miniand confirm that the new value doesn't cause layout issues:✅ Verification successful
Verification Successful: The new value of
--tv-Button-min-width-miniis appropriate and does not impact any existing button layouts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the new min-width value # Test: Search for all instances where `--tv-Button-min-width-mini` is used rg --type less --no-filename '--tv-Button-min-width-mini' # Expected Result: The new value should not negatively affect button layoutsLength of output: 486
Script:
#!/bin/bash # Description: Corrected script to verify the impact of the new min-width value # Test: Search for all instances where `--tv-Button-min-width-mini` is used rg --type less --no-filename -e '--tv-Button-min-width-mini' # Expected Result: List all occurrences to assess potential layout impactsLength of output: 124
packages/theme/src/button/test.less (2)
261-264: Ensure comments accurately describe the danger buttonIn line 261:
// 朴素类危险按钮文本色Verify that all comments within the danger button section correctly reference the "danger button" (危险按钮) and not other button types. Consistent and accurate comments help prevent misunderstandings.
386-386: Potential redundancy in padding variablesIn line 386:
--ti-button-size-normal-padding: var(--ti-common-space-6);This variable may overlap with
--ti-button-size-normal-padding-horizontaldefined earlier. Consider reviewing these variables to eliminate redundancy and ensure clarity.
| const res = suffix | ||
| .map((suf) => { | ||
| const sufx = suf.split('|') | ||
|
|
||
| return props.map((prop) => { | ||
| const names = prop.split('|') | ||
| return '// ' + sufx[1] + prefix[1] + names[1] + '\n' + prefix[0] + names[0] + sufx[0] + ': var(--xxxxx);\n' | ||
| }) | ||
| }) | ||
| .flat() | ||
| .join('') | ||
|
|
||
| /* eslint-disable no-console */ | ||
| console.log(res) |
There was a problem hiding this comment.
Consider alternative output methods and remove console.log
The current implementation uses console.log to output the generated CSS variables, which is typically discouraged in production code. Consider the following improvements:
- Replace
console.logwith a more appropriate output method, such as writing to a file or returning the generated string. - Remove the ESLint disable comment once console.log is no longer used.
Here's a suggested refactor:
- const res = suffix
+ const generateCSSVariables = () => suffix
.map((suf) => {
const sufx = suf.split('|')
return props.map((prop) => {
const names = prop.split('|')
return '// ' + sufx[1] + prefix[1] + names[1] + '\n' + prefix[0] + names[0] + sufx[0] + ': var(--xxxxx);\n'
})
})
.flat()
.join('')
- /* eslint-disable no-console */
- console.log(res)
+ return generateCSSVariables()This refactor encapsulates the generation logic in a function and removes the console.log, making the code more reusable and maintainable.
📝 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.
| const res = suffix | |
| .map((suf) => { | |
| const sufx = suf.split('|') | |
| return props.map((prop) => { | |
| const names = prop.split('|') | |
| return '// ' + sufx[1] + prefix[1] + names[1] + '\n' + prefix[0] + names[0] + sufx[0] + ': var(--xxxxx);\n' | |
| }) | |
| }) | |
| .flat() | |
| .join('') | |
| /* eslint-disable no-console */ | |
| console.log(res) | |
| const generateCSSVariables = () => suffix | |
| .map((suf) => { | |
| const sufx = suf.split('|') | |
| return props.map((prop) => { | |
| const names = prop.split('|') | |
| return '// ' + sufx[1] + prefix[1] + names[1] + '\n' + prefix[0] + names[0] + sufx[0] + ': var(--xxxxx);\n' | |
| }) | |
| }) | |
| .flat() | |
| .join('') | |
| return generateCSSVariables() |
| &.is-ghost.is-ghost.is-ghost.is-ghost { | ||
| background-color: var(--tv-Button-bg-color-ghost); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid repeating selectors to increase specificity.
Repeating the same class or attribute multiple times to increase CSS specificity (e.g., .is-ghost.is-ghost.is-ghost.is-ghost) can make the code harder to read and maintain. Consider refactoring to use more specific class names, leveraging the CSS cascade, or using !important judiciously if necessary.
Also applies to: 172-179
packages/theme/src/button/index.less
Outdated
| &.is-ghost.is-ghost.is-ghost.is-ghost { | ||
| background-color: var(--tv-Button-bg-color-ghost); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid increasing specificity by repeating class names.
Repeating the class selector .is-ghost multiple times to increase specificity can reduce code readability and maintainability. Consider using a more maintainable method to handle specificity, such as leveraging the CSS cascade, using more specific selectors, or reorganizing your styles.
| &.is-disabled.is-disabled.is-disabled.is-disabled.is-disabled, | ||
| &.is-loading.is-loading.is-loading.is-loading.is-loading, | ||
| &[disabled][disabled][disabled][disabled][disabled] { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to improve selector specificity without repetition.
Lines 171-173 repeat class selectors like .is-disabled and [disabled] multiple times to increase specificity. This practice can make the code harder to manage. Explore alternative approaches to achieve the desired specificity, such as restructuring your CSS or using !important judiciously.
| &.is-disabled.is-disabled.is-disabled.is-disabled, | ||
| &.is-loading.is-loading.is-loading.is-loading, | ||
| &[disabled][disabled][disabled][disabled][disabled][disabled] { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify selectors to enhance readability.
The selectors in lines 289-291 repeat classes multiple times, which can be confusing. Simplifying these selectors will improve code clarity and maintainability.
| &:has(.@{svg-prefix-cls}) { | ||
| padding: 0 var(--ti-button-text-svg-padding-horizontal); | ||
| color: var(--ti-button-text-has-svg-text-color); | ||
| } | ||
|
|
||
| &:has(.@{svg-prefix-cls}):hover { | ||
| background: var(--ti-button-text-hover-bg-color); | ||
| border-radius: var(--ti-button-text-hover-border-radius); | ||
| color: var(--ti-button-text-has-svg-hover-text-color); | ||
| & .@{svg-prefix-cls} { | ||
| fill: var(--ti-button-text-svg-hover-fill-color); | ||
| } | ||
| } | ||
|
|
||
| &:has(.@{svg-prefix-cls}).is-disabled { | ||
| background: transparent; | ||
| fill: var(--ti-button-text-svg-disabled-fill-color); | ||
| color: var(--ti-button-text-color-disabled); | ||
|
|
||
| & .@{svg-prefix-cls} { | ||
| fill: var(--ti-button-text-svg-disabled-fill-color); | ||
| } | ||
|
|
||
| &:hover { | ||
| fill: var(--ti-button-text-svg-disabled-fill-color); | ||
| background: transparent; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid Using :has() Pseudo-Class Due to Limited Browser Support
The :has() pseudo-class used in selectors like &:has(.@{svg-prefix-cls}) is not widely supported across all browsers. Currently, it is only supported in some versions of Safari. Relying on it may cause styles to not be applied in browsers like Chrome, Firefox, and Edge.
Consider refactoring the code to avoid using :has(). You can achieve similar styling effects by adding a parent class or restructuring the HTML to include additional classes that can be targeted without the :has() pseudo-class.
| .@{css-prefix-iconfont}-loading { | ||
| font-size: var(--ti-button-loading-font-size); | ||
| line-height: 1; | ||
| animation: rotating 2s linear infinite; | ||
| } |
There was a problem hiding this comment.
Duplicate Definitions of .@{css-prefix-iconfont}-loading Class
The class .@{css-prefix-iconfont}-loading is defined twice with different font-size values:
-
Lines 549-553:
.@{css-prefix-iconfont}-loading { font-size: var(--ti-button-loading-font-size); line-height: 1; animation: rotating 2s linear infinite; }
-
Lines 556-560:
.@{css-prefix-iconfont}-loading { font-size: var(--ti-common-font-size-1); line-height: 1; animation: rotating 2s linear infinite; }
This duplication can lead to conflicts and unexpected behavior in styling.
Consolidate these definitions into a single class definition to ensure consistent styling. Decide on the appropriate font-size and other properties, and remove the redundant class.
Also applies to: 556-560
| &&--primary { | ||
| .button-type( | ||
| var(--ti-button-primary-text-color), | ||
| var(--ti-button-primary-normal-bg-color), | ||
| var(--ti-button-primary-normal-border-color), | ||
| var(--ti-button-primary-hover-bg-color), | ||
| var(--ti-button-primary-hover-border-color), | ||
| var(--ti-button-primary-active-bg-color), | ||
| var(--ti-button-primary-disabled-text-color), | ||
| var(--ti-button-primary-disabled-bg-color), | ||
| var(--ti-button-primary-plain-text-color), | ||
| var(--ti-button-primary-plain-hover-text-color), | ||
| var(--ti-button-primary-plain-bg-color), | ||
| var(--ti-button-primary-plain-hover-bg-color), | ||
| var(--ti-button-primary-plain-border-color), | ||
| var(--ti-button-primary-plain-hover-border-color), | ||
| var(--ti-button-primary-plain-disabled-bg-color), | ||
| var(--ti-button-primary-disabled-border-color), | ||
| var(--ti-button-primary-ghost-text-color), | ||
| var(--ti-button-primary-ghost-hover-text-color), | ||
| var(--ti-button-primary-ghost-disabled-text-color), | ||
| var(--ti-button-primary-ghost-border-color), | ||
| var(--ti-button-primary-ghost-hover-border-color), | ||
| var(--ti-button-primary-ghost-disabled-border-color), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Refactoring to Reduce Code Duplication
The style blocks for button types (primary, success, warning, danger, info) repeatedly call the .button-type mixin with numerous parameters:
- Example from lines 259-284:
&&--primary { .button-type( var(--ti-button-primary-text-color), var(--ti-button-primary-normal-bg-color), // ... (additional variables) ); }
This repetition violates the DRY (Don't Repeat Yourself) principle and can make the code harder to maintain.
Consider refactoring by creating a map of button types and their corresponding variables. You can then iterate over this map using LESS's each function to generate the styles dynamically. This approach reduces redundancy and simplifies updates.
Also applies to: 286-311, 313-338, 340-365, 367-392
| line-height: 1; | ||
| padding: var(--ti-button-size-normal-padding); | ||
|
|
||
| &.is-hide-boder { |
There was a problem hiding this comment.
Typo in Class Name 'is-hide-boder'
There is a typo in the class name is-hide-boder; it should be is-hide-border.
Apply this fix:
- &.is-hide-boder {
+ &.is-hide-border {Ensure that you update any references to this class name throughout the codebase.
📝 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.
| &.is-hide-boder { | |
| &.is-hide-border { |
| &&--default.is-plain { | ||
| border-color: var(--ti-button-plain-border-color); | ||
| } | ||
| &&--default.is-disabled.is-plain { | ||
| border-color: var(--ti-button-plain-disabled-border-color); | ||
| } |
There was a problem hiding this comment.
Incorrect Usage of && in LESS Selectors
The use of && in selectors like &&--default may not produce the intended results. In LESS, & refers to the parent selector, and using && concatenates the parent selector twice, which could lead to incorrect CSS classes. To append modifiers to the parent selector, use &--modifier.
For example, replace &&--default with &--default to correctly target .button--default.
Apply the following fix and update similar instances throughout the file:
- &&--default.is-plain {
+ &--default.is-plain {This change should be applied consistently wherever && is used in selectors to ensure proper class targeting.
📝 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.
| &&--default.is-plain { | |
| border-color: var(--ti-button-plain-border-color); | |
| } | |
| &&--default.is-disabled.is-plain { | |
| border-color: var(--ti-button-plain-disabled-border-color); | |
| } | |
| &--default.is-plain { | |
| border-color: var(--ti-button-plain-border-color); | |
| } | |
| &--default.is-disabled.is-plain { | |
| border-color: var(--ti-button-plain-disabled-border-color); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/theme/src/button/index.less (2)
28-28: Translate code comments to English for consistencyThe comments on lines 28, 39, 67, 181, and 305 are in Chinese. To maintain consistency and improve collaboration among all team members, please translate them into English.
Also applies to: 39-39, 67-67, 181-181, 305-305
305-305: Translate code comment to English for clarityThe comment on line 305 is in Chinese. For consistency, please translate it into English so all team members can understand the context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/theme/src/button/index.less (1 hunks)
🔇 Additional comments (9)
packages/theme/src/button/index.less (9)
11-11: Verify that the mixin.inject-Button-vars()is properly defined and importedEnsure that the mixin
.inject-Button-vars()is correctly defined and imported. If it's defined in an external file, make sure that file is imported to prevent compilation errors.
70-78: Ensure CSS variables are correctly defined for theme mixinsIn the
.color-theme-mixin(@theme), variables like--tv-Button-text-color-@{theme}are referenced. Please verify that these CSS variables are correctly defined in yourvars.lessfile to prevent runtime issues.
92-102: Confirm the correctness of active state styles in mixinsThe
.color-active-mixin(@theme, @plain)defines styles for active states. Ensure that the variables and logic within this mixin correctly reflect the desired styling for different button states.
104-154: Validate consistency across different button variantsThe styles for button variants (
--default,--primary,--success,--warning,--danger,--info) use mixins for theming and active states. Verify that each variant displays as intended and that the class names correspond correctly to their styles.
183-190: Double-check size calculations in.size-only-icon-mixinThe mixin
.size-only-icon-mixin(@size)sets dimensions for icon-only buttons. Ensure that the variables interpolate correctly and that the resulting button sizes match design specifications.
194-197: Review the calculation in.size-only-icon-ontext-iconsize-mixinIn the mixin
.size-only-icon-ontext-iconsize-mixin(@size), the font size is calculated using:@wh: %('calc( var(--tv-Button-height%a) - 8px)', @size);Verify that this calculation produces the correct icon size and that
8pxis the intended value to subtract for all sizes.
306-329: Ensure consistency in styles for text buttons with iconsThe styles for
.&@{button-prefix-cls}--text.is-icondefine text buttons with icons. Verify that these styles align with design requirements, especially regarding padding, font size, and hover effects.
334-336: Check image alignment and marginThe
& > imgselector adds a right margin and vertical alignment to images within buttons. Ensure this does not conflict with other styles and that it works correctly across different button sizes and states.
340-345: Confirm margin adjustments between buttons of different sizesThe margins set for adjacent buttons of various sizes ensure appropriate spacing. Verify that these margins are consistent with design guidelines and appear correctly in the UI.
packages/theme/src/button/index.less
Outdated
| /* stylelint-disable no-duplicate-selectors */ | ||
| /* stylelint-disable function-no-unknown */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider configuring stylelint to recognize custom functions instead of disabling rules
Disabling no-duplicate-selectors and function-no-unknown may hide potential issues in your code. If you're using custom functions or intentionally have duplicate selectors, consider configuring your stylelint settings to recognize these patterns. This approach maintains linting benefits while accommodating your codebase's specifics.
| .size-mixin(@size) { | ||
| @fs: %('var(--tv-Button-font-size%a)', @size); | ||
| @h: %('var(--tv-Button-height%a)', @size); | ||
| @p: %('var(--tv-Button-padding-x%a)', @size); | ||
| @mw: %('var(--tv-Button-min-width%a)', @size); | ||
|
|
||
| &:hover { | ||
| border-color: var(--ti-button-only-icon-disabled-border-color); | ||
| } | ||
| } | ||
| } | ||
| &.is-only-icon.is-circle { | ||
| width: unset; | ||
| font-size: e(@fs); | ||
| height: e(@h); | ||
| padding: 0 e(@p); | ||
| min-width: e(@mw); |
There was a problem hiding this comment.
Verify the usage of the %() function in variable interpolation
The %() function is used for variable interpolation in the mixin .size-mixin(@size). For example:
@fs: %('var(--tv-Button-font-size%a)', @size);This syntax may not be standard in Less and could lead to errors during compilation. Please ensure that this function is defined or supported in your build process.
| &.is-only-icon.is-only-icon { | ||
| // 只有default主题的边框色和 hover 边框色 是和主题色相反的。其它都随主题的色。 | ||
| // 仅图标的plain场景，无对应示例，不处理。 | ||
| &.@{button-prefix-cls}--default { | ||
| border-color: var(--tv-Button-border-color-only-icon-default); | ||
|
|
||
| &.is-disabled:hover { | ||
| text-decoration: none; | ||
| &:active, | ||
| &:focus, | ||
| &:hover, | ||
| &.is-active { | ||
| border-color: var(--tv-Button-border-color-only-icon-default-hover); | ||
| } | ||
| } | ||
|
|
||
| border-radius: var(--ti-button-text-hover-border-radius); | ||
| padding: 0; //不需要padding | ||
|
|
||
| padding: 0 var(--ti-button-text-padding-horizontal); | ||
| .size-only-icon-mixin(e('')); // 不同尺寸时，大小不同 | ||
|
|
||
| &:has(.@{svg-prefix-cls}) { | ||
| padding: 0 var(--ti-button-text-svg-padding-horizontal); | ||
| color: var(--ti-button-text-has-svg-text-color); | ||
| &.@{button-prefix-cls}--large { | ||
| .size-only-icon-mixin(-large); | ||
| } | ||
| &.@{button-prefix-cls}--medium { | ||
| .size-only-icon-mixin(-medium); | ||
| } | ||
| &.@{button-prefix-cls}--small { | ||
| .size-only-icon-mixin(-small); | ||
| } | ||
| &.@{button-prefix-cls}--mini { | ||
| .size-only-icon-mixin(-mini); | ||
| } | ||
|
|
||
| &:has(.@{svg-prefix-cls}):hover { | ||
| background: var(--ti-button-text-hover-bg-color); | ||
| border-radius: var(--ti-button-text-hover-border-radius); | ||
| color: var(--ti-button-text-has-svg-hover-text-color); | ||
| & .@{svg-prefix-cls} { | ||
| fill: var(--ti-button-text-svg-hover-fill-color); | ||
| } | ||
| &:not(.is-circle) { | ||
| border-radius: var(--tv-Button-border-radius-only-icon); | ||
| } | ||
|
|
||
| &:has(.@{svg-prefix-cls}).is-disabled { | ||
| background: transparent; | ||
| fill: var(--ti-button-text-svg-disabled-fill-color); | ||
| color: var(--ti-button-text-color-disabled); | ||
| // 无边框时： 图标大小为button的大小的高度 | ||
| &.@{button-prefix-cls}--text { | ||
| border: none; | ||
| border-radius: var(--tv-Button-border-radius-only-icon-ontext); // 影响hover时的阴影 | ||
|
|
||
| & .@{svg-prefix-cls} { | ||
| fill: var(--ti-button-text-svg-disabled-fill-color); | ||
| } | ||
| background-color: var(--tv-Button-bg-color-only-icon-ontext); | ||
|
|
||
| &:hover { | ||
| fill: var(--ti-button-text-svg-disabled-fill-color); | ||
| background: transparent; | ||
| &:active, | ||
| &:focus, | ||
| &:hover, | ||
| &.is-active { | ||
| background-color: var(--tv-Button-bg-color-only-icon-ontext-hover); | ||
| } | ||
| } | ||
| // 不同的尺寸，icon大小 为高度。 原型稿不包含 | ||
| .size-only-icon-ontext-iconsize-mixin(e('')); | ||
|
|
||
| &.is-only-icon { | ||
| width: var(--ti-button-only-icon-text-width); | ||
| &:hover { | ||
| background: var(--ti-button-text-only-icon-hover-bg-color); | ||
| &.@{button-prefix-cls}--large { | ||
| .size-only-icon-ontext-iconsize-mixin(-large); | ||
| } | ||
| &.@{button-prefix-cls}--medium { | ||
| .size-only-icon-ontext-iconsize-mixin(-medium); | ||
| } | ||
| &.@{button-prefix-cls}--small { | ||
| .size-only-icon-ontext-iconsize-mixin(-small); | ||
| } | ||
| &.@{button-prefix-cls}--mini { | ||
| .size-only-icon-ontext-iconsize-mixin(-mini); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid increasing specificity by repeating class selectors
Using selectors like &.is-only-icon.is-only-icon to increase specificity can reduce maintainability. Consider alternative methods such as using !important, restructuring your CSS, or adjusting the order of your styles to achieve the desired specificity without repetition.
shenjunjian
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/theme/src/button/vars.less (2)
242-291: LGTM: Icon and text combination variables are comprehensiveThe variables for various icon and text combinations are well-defined and cover a wide range of scenarios. The comments provide clear explanations for each variable and scenario, enhancing maintainability.
For consistency, consider adding a space after the
/in the comment on line 282:- //混排无边框时，字体大小 + // 混排无边框时，字体大小This minor change will align the comment style with the rest of the file.
Line range hint
1-291: Overall: Excellent refactoring of button variablesThe refactoring of button variables in this file has significantly improved the organization, clarity, and maintainability of the code. The consistent naming convention using the
--tv-Button-prefix and the use of existing theme variables promote overall theme consistency.To further enhance code readability and collaboration:
Consider translating all remaining Chinese comments to English. This will ensure that all contributors can easily understand the purpose and context of each variable.
Implement the suggested improvements for button padding and width calculations using more maintainable expressions, as mentioned in the earlier comment.
These minor improvements will make the code even more accessible and easier to maintain for all team members.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/sites/demos/pc/app/button/ghost.spec.ts (1 hunks)
- packages/theme/src/button/index.less (1 hunks)
- packages/theme/src/button/vars.less (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/button/index.less
🔇 Additional comments (11)
examples/sites/demos/pc/app/button/ghost.spec.ts (7)
36-36: LGTM: Success ghost button color transition is consistent.The border-bottom-color change for the success ghost button follows the pattern established by other button types, transitioning from a lighter color to a darker one upon interaction.
To ensure these color values align with the design system, please run the following command:
#!/bin/bash # Search for success color definitions in the design system rg -i "success.*color|color.*success" --type ts --type tsx --type cssAlso applies to: 42-42
23-25: LGTM: Secondary ghost button color changes are consistent.The color and border-bottom-color changes for the secondary ghost button appear to be intentional and consistent with the primary button behavior. The text color is now darker, which may improve readability.
To ensure consistency across all button types, please run the following command to check for any discrepancies:
#!/bin/bash # Search for all ghost button color definitions in the codebase rg -i "ghost.*button.*color|button.*ghost.*color" --type ts --type tsx --type cssAlso applies to: 29-31
47-47: LGTM: Info ghost button color transition is consistent.The border-bottom-color change for the info ghost button follows the pattern established by other button types, transitioning from a lighter color to a darker one upon interaction.
To ensure these color values align with the design system, please run the following command:
#!/bin/bash # Search for info color definitions in the design system rg -i "info.*color|color.*info" --type ts --type tsx --type cssAlso applies to: 53-53
58-58: LGTM: Warning ghost button color transition is consistent.The border-bottom-color change for the warning ghost button follows the pattern established by other button types, transitioning from a lighter color to a darker one upon interaction.
To ensure these color values align with the design system, please run the following command:
#!/bin/bash # Search for warning color definitions in the design system rg -i "warning.*color|color.*warning" --type ts --type tsx --type cssAlso applies to: 64-64
69-69: LGTM: Danger ghost button color transition is consistent.The border-bottom-color change for the danger ghost button follows the pattern established by other button types, transitioning from a lighter color to a darker one upon interaction.
To ensure these color values align with the design system, please run the following command:
#!/bin/bash # Search for danger color definitions in the design system rg -i "danger.*color|color.*danger" --type ts --type tsx --type cssAlso applies to: 75-75
Line range hint
1-77: Overall LGTM: Ghost button color transitions are consistent and well-tested.The changes in this file improve the consistency of ghost button behavior across all types (primary, secondary, success, info, warning, and danger). Each button now transitions from a lighter border color to a darker one upon interaction, which is a common pattern for indicating button state.
To ensure complete consistency and alignment with the design system, consider the following:
- Run all the verification commands provided in the individual comments to check color definitions and design specifications.
- Review the entire button component to ensure these changes are reflected in the implementation as well as the tests.
- Update any relevant documentation or style guides to reflect these new color transitions for ghost buttons.
To perform a comprehensive check of the entire button component, run the following command:
#!/bin/bash # Search for all button-related files in the codebase fd -e ts -e tsx -e css -e scss buttonThis will help you identify all files related to the button component, allowing you to ensure that the changes are consistently applied across the entire component.
19-19: LGTM: Primary ghost button color change looks good.The border-bottom-color change for the primary ghost button after clicking seems intentional and follows a common pattern for indicating button state.
To ensure this change aligns with the design specifications, please run the following command to check for any related design documents:
packages/theme/src/button/vars.less (4)
15-26: LGTM: Global button variables are well-definedThe global button variables for font weight, border width, and border radius are clearly defined with consistent naming conventions and helpful comments. The use of existing variables promotes consistency across the theme.
70-126: LGTM: Button color variables are comprehensive and well-organizedThe color variables for different button states and themes (default, primary, success, warning, danger, and info) are clearly defined. The consistent naming convention and use of existing color variables promote overall theme consistency. The comments provide clear explanations for each variable, enhancing maintainability.
130-173: LGTM: Plain and ghost button variables are well-definedThe variables for plain and ghost button styles are clearly defined for different themes. The use of existing color variables maintains consistency with the overall theme. The transparent background for ghost buttons is correctly implemented.
177-240: LGTM: Active and disabled button variables are comprehensiveThe variables for active and disabled button states are well-defined for different themes. The structure follows the established pattern, and the comments provide clear explanations. The disabled state variables are correctly defined with a higher priority, ensuring consistent behavior across all button types.
| @@ -9,438 +9,284 @@ | |||
| * A PARTICULAR PURPOSE. SEE THE APPLICABLE LICENSES FOR MORE DETAILS. | |||
| * | |||
| */ | |||
| .inject-Button-vars() { | |||
| //-------------------------------------------------------全局场景：------------------------------------------ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider translating the comment to English for consistency
The comment on line 13 is in Chinese. To maintain consistency and improve collaboration among all contributors, it's recommended to use English for code comments.
Consider translating the comment to English. For example:
- //-------------------------------------------------------全局场景：------------------------------------------
+ //------------------------------------------------------- Global scenarios: ------------------------------------------📝 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.
| //-------------------------------------------------------全局场景：------------------------------------------ | |
| //------------------------------------------------------- Global scenarios: ------------------------------------------ |
| --tv-Button-padding-x: 24px; // space变量不存在，是否替换成 space-base * 6 这种？ | ||
| // 默认时按钮最小宽度 | ||
| --tv-Button-min-width: 96px; // 是否替换成 size-base * 24 这种？ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing suggested improvements for button padding and width
The comments on lines 34 and 36 suggest potential improvements for defining button padding and minimum width using more maintainable expressions.
Consider implementing these suggestions:
- For padding:
- --tv-Button-padding-x: 24px; // space变量不存在，是否替换成 space-base * 6 这种？
+ --tv-Button-padding-x: calc(var(--tv-space-base) * 6);- For minimum width:
- --tv-Button-min-width: 96px; // 是否替换成 size-base * 24 这种？
+ --tv-Button-min-width: calc(var(--tv-size-base) * 24);This approach would make the values more flexible and easier to maintain across different themes or screen sizes.
📝 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.
| --tv-Button-padding-x: 24px; // space变量不存在，是否替换成 space-base * 6 这种？ | |
| // 默认时按钮最小宽度 | |
| --tv-Button-min-width: 96px; // 是否替换成 size-base * 24 这种？ | |
| --tv-Button-padding-x: calc(var(--tv-space-base) * 6); | |
| // 默认时按钮最小宽度 | |
| --tv-Button-min-width: calc(var(--tv-size-base) * 24); |
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
使用场景的方法，重构button的样式
snippet.js暂时保留，待以后删除！
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation