fix(button): add ghost's cssvars for button #2235
Conversation
WalkthroughThe changes in this pull request involve modifications to several LESS files related to UI components. Key updates include the addition of new CSS custom properties for ghost button states in Changes
Possibly related PRs
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 (
|
|
[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: 4
🧹 Outside diff range and nitpick comments (3)
packages/theme/src/base/reset.less (2)
37-41: Approve loading icon styles with a minor suggestionThe addition of the loading icon styles is good and aligns with the PR objectives. The use of the
@{css-prefix-iconfont}variable is a good practice for maintainability.Consider adding a fallback for browsers that don't support animations:
.@{css-prefix-iconfont}-loading { line-height: 1; + display: inline-block; /* Fallback for non-animated version */ animation: rotating 2s linear infinite; }This ensures that the icon is still visible and properly aligned even if the animation doesn't work.
43-51: Approve rotating animation with performance optimization suggestionThe
rotatingkeyframes are well-defined and will create a smooth rotation animation.For better performance, consider using the
transformproperty withrotateinstead ofrotateZ:@keyframes rotating { 0% { - transform: rotateZ(0deg); + transform: rotate(0deg); } 100% { - transform: rotateZ(360deg); + transform: rotate(360deg); } }This change simplifies the transform and may lead to slightly better performance, especially on mobile devices.
packages/theme/src/button/vars.less (1)
180-180: Avoid informal comments regarding design uncertaintiesThe comments in lines 180 and 229 mention that "maybe the designer got it wrong" (
可能设计师搞错了). It's best to avoid such informal remarks in the codebase. Instead, consider rephrasing the comments to professionally indicate the issue.Consider updating the comments as follows:
- // 没有这个边框色，可能设计师搞错了。 + // Border color variable not defined; please confirm with the design team.- // 没有相应的css变量,可能设计师搞错了 + // No corresponding CSS variable; please verify with the design team.Also applies to: 229-229
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/theme/src/base/reset.less (2 hunks)
- packages/theme/src/button/index.less (2 hunks)
- packages/theme/src/button/vars.less (7 hunks)
- packages/theme/src/float-button/index.less (6 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/float-button/index.less
🧰 Additional context used
🔇 Additional comments (11)
packages/theme/src/base/reset.less (2)
Line range hint
1-51: Summary of changes in reset.lessThe changes to this file successfully introduce the loading icon styles and animation as part of the ghost button implementation. The additions are well-structured and consistent with the existing codebase. A few minor suggestions were made to improve maintainability and performance:
- Consider auditing and potentially deprecating unused icon styles.
- Add a fallback for the loading icon in non-animated contexts.
- Optimize the rotation animation for better performance.
These changes contribute positively to the UI framework and align well with the PR objectives.
Line range hint
16-35: Address the TODO comment regarding default icon stylesThe added comment raises a valid question about the necessity of retaining these default icon styles. To improve code maintainability and reduce potential CSS bloat, consider the following steps:
- Audit the usage of these icon classes across the project.
- If they are not used or can be replaced with more modern styling approaches (e.g., CSS variables), consider deprecating and eventually removing them.
- If they are still needed, document the reason for keeping them to prevent future confusion.
To help with the audit, you can run the following script to check for usage of these icon classes:
This will help determine if these styles are still necessary or can be safely removed.
packages/theme/src/button/index.less (9)
88-95: [Approved] Added Ghost Button Mixin.color-ghost-mixin(@theme)The new mixin
.color-ghost-mixin(@theme)is correctly implemented to define the text and border colors for ghost buttons based on the theme. The usage of variables aligns with the existing pattern in the codebase.
99-99: [Approved] Corrected Variable Name in.color-active-mixinThe variable
@borderhas been correctly updated to use@border: %('var(--tv-Button-border-color%a-active-%a)', @plain, @theme);, ensuring consistency with the naming conventions and proper application of border colors in the active state.
109-119: [Approved] Added Ghost Button Active State Mixin.color-ghost-active-mixin(@theme)The new mixin
.color-ghost-active-mixin(@theme)properly handles the active, focus, hover, and active class states for ghost buttons by updating the border color based on the theme. This addition enhances the button's responsiveness to user interactions.
125-133: [Approved] Integrated Ghost Button Styles into Default ThemeThe ghost button styles have been correctly integrated into the default theme by adding the
&.is-ghostclass and applying the appropriate mixins.color-ghost-mixin(default);and.color-ghost-active-mixin(default);. This ensures consistent styling for ghost buttons in the default theme.
139-147: [Approved] Integrated Ghost Button Styles into Primary ThemeSimilarly, ghost button styles have been integrated into the primary theme. The use of
&.is-ghostwith the respective mixins applies the correct styles for ghost buttons in the primary color scheme.
153-161: [Approved] Integrated Ghost Button Styles into Success ThemeThe ghost button styles are appropriately added to the success theme, ensuring that ghost buttons reflect the success color scheme with the correct active states.
167-175: [Approved] Integrated Ghost Button Styles into Warning ThemeThe addition of ghost button styles to the warning theme is correctly implemented, maintaining consistency across different button variants.
181-189: [Approved] Integrated Ghost Button Styles into Danger ThemeGhost button styles have been properly added to the danger theme, allowing for consistent styling of ghost buttons in this variant.
195-203: [Approved] Integrated Ghost Button Styles into Info ThemeThe integration of ghost button styles into the info theme is correctly done, ensuring that all button themes support the ghost variant.
| // ghost：bg 强制为透明，比禁用还高 优先级为：六级类名 | ||
| &.is-ghost.is-ghost.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
Refactor to Reduce Selector Specificity
The selector &.is-ghost.is-ghost.is-ghost.is-ghost.is-ghost.is-ghost uses six repetitions of .is-ghost to increase specificity. This approach can reduce maintainability and readability of the code.
Consider refactoring to reduce the specificity without relying on repeated class names. One option is to restructure the CSS to avoid the need for such high specificity or to use more specific parent selectors if applicable.
For example, you could adjust the CSS hierarchy or use a more specific attribute selector:
- &.is-ghost.is-ghost.is-ghost.is-ghost.is-ghost.is-ghost {
+ &.button-container &.is-ghost {Or, if appropriate, introduce a modifier class that explicitly indicates the higher specificity:
&.is-ghost-high {
background-color: var(--tv-Button-bg-color-ghost);
}And apply this class where needed in the HTML:
<button class="tiny-button is-ghost is-ghost-high">Ghost Button</button>This approach improves code maintainability by avoiding excessive class chaining.
| --tv-Button-border-color-ghost-default: #808080; // 没有这个边框色，可能设计师搞错了。 | ||
|
|
||
| // primary 主题时按钮幽灵文本色 | ||
| --tv-Button-text-colorghostn-primary: var(--tv-color-act-primary-text); |
There was a problem hiding this comment.
Fix typo in variable name --tv-Button-text-colorghostn-primary
There's a typo in the variable name --tv-Button-text-colorghostn-primary. It should be --tv-Button-text-color-ghost-primary to maintain consistency with the naming convention and other variable names.
Apply this diff to correct the variable name:
- --tv-Button-text-colorghostn-primary: var(--tv-color-act-primary-text);
+ --tv-Button-text-color-ghost-primary: var(--tv-color-act-primary-text);📝 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-text-colorghostn-primary: var(--tv-color-act-primary-text); | |
| --tv-Button-text-color-ghost-primary: var(--tv-color-act-primary-text); |
packages/theme/src/button/vars.less
Outdated
| // 默认时按钮幽灵文本色 | ||
| --tv-Button-text-color-ghost-default: var(--tv-color-text-secondary); | ||
| // 默认时按钮幽灵边框色 | ||
| --tv-Button-border-color-ghost-default: #808080; // 没有这个边框色，可能设计师搞错了。 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded color #808080 with a CSS variable
The variable --tv-Button-border-color-ghost-default uses a hardcoded color #808080. For consistency and theming flexibility, consider defining and using a CSS variable for this color.
Apply this diff to replace the hardcoded color:
- --tv-Button-border-color-ghost-default: #808080; // 没有这个边框色，可能设计师搞错了。
+ --tv-Button-border-color-ghost-default: var(--tv-color-border-ghost-default); // Define this variable accordingly.If --tv-color-border-ghost-default does not exist, please add it to the variables list or consult with the design team to determine the appropriate value.
Committable suggestion was skipped due to low confidence.
packages/theme/src/button/vars.less
Outdated
| @@ -191,6 +225,8 @@ | |||
| --tv-Button-bg-color-plain-active-primary: var(--tv-color-act-primary-bg-white-active); | |||
| // primary 主题时按钮激活的朴素边框色 | |||
| --tv-Button-border-color-plain-active-primary: var(--tv-color-act-primary-bg-white-active); // 无边框 | |||
| // primary 主题时按钮激活的幽灵边框色 | |||
| --tv-Button-border-color-ghost-active-primary: #dbdbdb; // 没有相应的css变量,可能设计师搞错了 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded color #dbdbdb with a CSS variable
The variable --tv-Button-border-color-ghost-active-primary uses a hardcoded color #dbdbdb. To maintain consistency and ease of theming, consider defining and using a CSS variable for this color.
Apply this diff to replace the hardcoded color:
- --tv-Button-border-color-ghost-active-primary: #dbdbdb; // 没有相应的css变量,可能设计师搞错了
+ --tv-Button-border-color-ghost-active-primary: var(--tv-color-border-ghost-active-primary); // Define this variable accordingly.If --tv-color-border-ghost-active-primary does not exist, please add it to the variables or coordinate with the design team to determine the correct value.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/button/vars.less (1)
Line range hint
1-324: Overall assessment: Good additions with minor improvements neededThe changes to this file significantly enhance the button styling system by adding support for ghost buttons and their various states. This improvement allows for more flexible and consistent button styling across the application. However, there are a few minor issues to address:
- Fix the typo in the variable name on line 183.
- Replace hardcoded color values with CSS variables for better maintainability and theming flexibility.
- Consult with the design team to clarify and potentially define new color variables for some of the hardcoded values.
Once these issues are addressed, the changes will greatly improve the button component's styling capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/theme/src/button/vars.less (7 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/theme/src/button/vars.less (5)
172-173: LGTM: Clear explanation of ghost and plain button relationshipThe comment clearly explains the relationship between ghost and plain button states, which is helpful for developers working with these styles.
228-229: LGTM: Consistent naming for ghost button active state variablesThe naming convention for ghost button active state variables is consistent across all themes (primary, success, warning, danger, info). This promotes good maintainability and readability of the code.
Also applies to: 239-240, 250-251, 261-262, 272-273
Line range hint
276-281: LGTM: Comprehensive disabled state handlingThe disabled state variables are well-defined and cover both regular and ghost button states. The comment explaining the high priority of the disabled state is helpful for understanding the styling hierarchy.
183-183:⚠️ Potential issueFix typo in variable name
There's a typo in the variable name
--tv-Button-text-colorghostn-primary. It should be--tv-Button-text-color-ghost-primaryto maintain consistency with other variable names.Apply this diff to correct the variable name:
- --tv-Button-text-colorghostn-primary: var(--tv-color-act-primary-text); + --tv-Button-text-color-ghost-primary: var(--tv-color-act-primary-text);Likely invalid or redundant comment.
177-180: 🛠️ Refactor suggestionReplace hardcoded color value with a CSS variable
The variable
--tv-Button-border-color-ghost-defaultuses a hardcoded color#808080. For consistency and easier theming, consider using a CSS variable instead.Apply this diff to replace the hardcoded color:
- --tv-Button-border-color-ghost-default: #808080; // 没有这个边框色，可能设计师搞错了。 + --tv-Button-border-color-ghost-default: var(--tv-color-border-ghost-default); // Define this variable accordinglyAlso, please consult with the design team to clarify the correct border color for the ghost default state.
Likely invalid or redundant comment.
packages/theme/src/button/vars.less
Outdated
| --tv-Button-border-color-plain-active-default: var(--tv-color-bg); // 朴素激活都无边框，所以等于背景色。 | ||
| // 默认时按钮激活的幽灵边框色 | ||
| --tv-Button-border-color-ghost-active-default: #dbdbdb; // 暂无对应变量 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded color value with a CSS variable
The variable --tv-Button-border-color-ghost-active-default uses a hardcoded color #dbdbdb. For consistency and easier theming, consider using a CSS variable instead.
Apply this diff to replace the hardcoded color:
- --tv-Button-border-color-ghost-active-default: #dbdbdb; // 暂无对应变量
+ --tv-Button-border-color-ghost-active-default: var(--tv-color-border-ghost-active-default); // Define this variable accordinglyPlease work with the design team to define an appropriate variable for this color.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/theme/src/base/vars.less (2)
345-346: LGTM! Consider a minor naming improvement.The addition of these ghost button border color variables aligns well with the PR objectives. They provide specific styling for the ghost button variant, which was mentioned as a goal in the PR description.
A small suggestion for consistency:
Consider renaming these variables to match the naming pattern of other button-related variables in the file. For example:
- --tv-color-border-ghost: var(--tv-base-color-common-8); - --tv-color-border-ghost-active: var(--tv-base-color-common-6); + --tv-color-act-ghost-border: var(--tv-base-color-common-8); + --tv-color-act-ghost-border-active: var(--tv-base-color-common-6);This would align with other button-related variables like
--tv-color-act-primary-borderand--tv-color-act-primary-border-active.
345-346: Consider adding a brief comment for the new variablesThe new ghost button border color variables fit well within the existing structure of the file. To maintain the high level of documentation present throughout the file:
Consider adding a brief comment explaining the purpose of these new ghost button variables. For example:
+ /* Ghost button border colors */ --tv-color-border-ghost: var(--tv-base-color-common-8); --tv-color-border-ghost-active: var(--tv-base-color-common-6);This will help maintain consistency with the rest of the file and provide clarity for other developers who may work with these variables in the future.
packages/theme/src/button/vars.less (1)
172-172: Clarify the comment regarding style priorityThe comment states that ghost and plain styles are at the same level, and if they coexist, the priority cannot guarantee stability. Consider elaborating on this to guide developers on how to handle scenarios where both styles are applied simultaneously, which will help prevent potential styling conflicts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/base/vars.less (1 hunks)
- packages/theme/src/button/vars.less (7 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/theme/src/base/vars.less (2)
Line range hint
1-346: Summary of changes and suggestionsThe changes to this file successfully introduce the necessary variables for ghost button styling, aligning with the PR objectives. Here's a summary of the review:
- The new variables for ghost button border colors have been added correctly.
- A suggestion was made to rename the variables to match the existing naming convention for button-related variables.
- A verification step was proposed to ensure the new variables are being used correctly in the button component styles.
- A recommendation was made to add a brief comment explaining the purpose of the new ghost button variables.
Overall, these changes enhance the design system by providing specific styling options for ghost buttons, as intended. The suggestions provided aim to improve consistency and clarity within the codebase.
345-346: Verify usage in button component stylesTo ensure these new variables are properly implemented:
Please run the following script to check for the usage of these new ghost button border color variables in the button component styles:
This will help confirm that the new variables are being used in the appropriate button component styles, ensuring the ghost button styling is applied as intended.
✅ Verification successful
Usage of new ghost button border color variables confirmed
The new variables
--tv-color-border-ghostand--tv-color-border-ghost-activeare correctly used within the button component styles:
packages/theme/src/button/vars.lessThis ensures that the ghost button styling is applied as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of new ghost button border color variables in button component styles # Search for the new variable names in .less files echo "Searching for --tv-color-border-ghost usage:" rg --type less -A 5 $'--tv-color-border-ghost' echo "Searching for --tv-color-border-ghost-active usage:" rg --type less -A 5 $'--tv-color-border-ghost-active' # If the suggested naming convention is adopted, also search for those echo "Searching for --tv-color-act-ghost-border usage (if renamed):" rg --type less -A 5 $'--tv-color-act-ghost-border' echo "Searching for --tv-color-act-ghost-border-active usage (if renamed):" rg --type less -A 5 $'--tv-color-act-ghost-border-active'Length of output: 2423
Script:
#!/bin/bash # Description: Corrected search for usage of new ghost button border color variables in button component styles # Search for the new variable names in .less files echo "Searching for --tv-color-border-ghost usage:" rg --type less -A 5 -- '--tv-color-border-ghost' echo "Searching for --tv-color-border-ghost-active usage:" rg --type less -A 5 -- '--tv-color-border-ghost-active' # If the suggested naming convention is adopted, also search for those echo "Searching for --tv-color-act-ghost-border usage (if renamed):" rg --type less -A 5 -- '--tv-color-act-ghost-border' echo "Searching for --tv-color-act-ghost-border-active usage (if renamed):" rg --type less -A 5 -- '--tv-color-act-ghost-border-active'Length of output: 3308
packages/theme/src/button/vars.less (2)
183-183: Fix typo in variable name--tv-Button-text-colorghostn-primaryThe typo previously mentioned is still present. The variable name
--tv-Button-text-colorghostn-primaryshould be corrected to--tv-Button-text-color-ghost-primaryto maintain consistency with the naming convention used throughout the file.
177-206: Ghost button variables are well-defined and consistentThe newly introduced CSS variables for the ghost button state are appropriately defined and consistently named. This enhancement aligns with the existing styling conventions and improves the theming capabilities for ghost buttons across different themes.
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
根据设计规范， ghost的边框 与plain完全不一样，需要新增一下ghost的相应变量值。
目前，ghost 态与plain态是平级的两个叠加状态。 它俩的类名优先级完全一样。
Summary by CodeRabbit
New Features
.tiny-icon-loadingclass.Bug Fixes
Chores
Style