refactor(theme): [button-group] refactor button-group theme vars #2276
Conversation
WalkthroughThe changes in this pull request involve modifications to the button group component across various files. Key updates include the removal of specific properties and events from the component's definition, adjustments to test cases reflecting these removals, and the introduction of new instances of button groups in templates. Additionally, there are significant refactoring efforts in the LESS stylesheets, including variable renaming and the introduction of new variables for styling consistency. Some files were deleted, indicating a cleanup of unused components and tests. Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
examples/sites/demos/pc/app/button-group/slot-default.spec.ts (1)
Line range hint
1-1: Consider translating the Chinese comment to English.The test description "测试按钮默认插槽" is in Chinese. For consistency and to improve maintainability for a potentially diverse team, consider translating this to English.
Suggested translation:
-test('测试按钮默认插槽', async ({ page }) => { +test('Test button default slot', async ({ page }) => {examples/sites/demos/pc/app/button-group/size-composition-api.vue (1)
Line range hint
1-8: LGTM! Consider adding a comment for the default-sized button group.The addition of a button group without a specified size attribute effectively demonstrates the default sizing. This change enhances the demo by showing a complete range of button group sizes.
To improve clarity, consider adding a comment above the new button group to explicitly state that it's using the default size:
<!-- Default size --> <tiny-button-group :data="groupData" v-model="checkedVal"></tiny-button-group>examples/sites/demos/pc/app/button-group/size.vue (1)
4-4: LGTM! Consider adding a comment for clarity.The addition of a button group without a specified size is a good way to demonstrate the default size. This change enhances the example by showing all available size options.
For improved clarity, consider adding a comment above this line to explicitly state that this represents the default size:
<!-- Default size --> <tiny-button-group :data="groupData" v-model="checkedVal"></tiny-button-group>examples/sites/demos/pc/app/button-group/size.spec.ts (3)
7-9: LGTM! Consider updating documentation.The changes to button height constants are consistent and introduce a new
defaultHeight. This refactoring aligns with the PR objectives to refactor theme variables.Consider updating the component documentation to reflect these new size definitions, ensuring users are aware of the changes in button dimensions.
21-22: LGTM! Consider adding a comment for clarity.The test assertions have been correctly updated to reflect the new button size definitions.
Consider adding a brief comment above these assertions to explain the different size categories being tested. This would improve the test's readability and maintainability.
23-23: LGTM! Consider enhancing test robustness.The addition of this assertion completes the test coverage for all button sizes, including the mini size.
To enhance the test's robustness, consider using a loop or parameterized test to check all button sizes. This would make it easier to add or modify size tests in the future. For example:
const sizes = [ { name: 'medium', height: mediumHeight }, { name: 'default', height: defaultHeight }, { name: 'small', height: smallHeight }, { name: 'mini', height: miniHeight } ]; for (let i = 0; i < sizes.length; i++) { await expect(demo.getByRole('button', { name: 'Button1' }).nth(i)).toHaveCSS('height', sizes[i].height); }examples/sites/demos/pc/app/button-group/disabled.spec.ts (2)
13-14: LGTM! Consider documenting color changes.The CSS property changes for the first button in
buttonGroup1are consistent with the refactoring of theme variables. The new colors (background: rgb(219, 219, 219), text: rgb(194, 194, 194)) appear to be appropriate for a disabled button state.Consider documenting these color changes in the component's documentation or in a changelog to help users understand the visual updates.
Line range hint
1-30: Summary of changes and potential impactThe changes in this file reflect a comprehensive update to the button group theme variables, affecting both enabled and disabled button states. The modifications include:
- Removal of the second button group from the test.
- Updates to background and text colors for disabled buttons.
- A change in the enabled button color for
buttonGroup3.These changes are likely to impact the visual appearance of button groups across the application. While the changes appear to be intentional and consistent, it's important to:
- Ensure that the new colors meet accessibility standards, particularly for color contrast.
- Update any related documentation or design guidelines to reflect these changes.
- Verify that these changes are consistently applied across all instances of button groups in the application.
Consider creating a centralized theme file or design token system (if not already in place) to manage these color values. This would make it easier to maintain consistency and make future updates across the entire application.
packages/theme/src/carousel/vars.less (1)
21-21: LGTM! Consider adding a comment for clarity.The update to
--tv-Carousel-arrow-hover-bg-colorlooks good. This change likely improves the visual consistency of the carousel component with the overall theme.Consider adding a brief comment explaining the rationale behind this specific color change. For example:
// Updated to improve contrast and align with the new design system --tv-Carousel-arrow-hover-bg-color: var(--tv-color-bg-hover-2);This would help future maintainers understand the reasoning behind this specific color choice.
packages/vue/src/button-group/src/pc.vue (1)
79-79: Approved: Removal of fixed width improves flexibilityThe removal of the fixed width (200) from the
<tiny-popover>component is a good change. This allows the popover to adapt to its content size, which is generally a more flexible approach.Consider adding a custom CSS class to the popover for easier styling if needed:
- <tiny-popover :visible-arrow="false" popper-class="tiny-group-item__more-popover"> + <tiny-popover :visible-arrow="false" popper-class="tiny-group-item__more-popover tiny-group-item__custom-popover">This way, you can easily target this specific popover for custom styling in your CSS if required in the future.
packages/theme/src/base/vars.less (1)
327-328: Ensure consistent naming conventionThe naming convention for the new variables (
--tv-color-bg-hover-2and--tv-color-bg-hover-3) follows the existing pattern. However, consider if more descriptive names would be beneficial, especially for--tv-color-bg-hover-3which is described as a general hover background color.For example:
- --tv-color-bg-hover-2: var(--tv-base-color-common-5); // #e6e6e6 轮播箭头背景悬浮色 - --tv-color-bg-hover-3: var(--tv-base-color-brand-1); // #f0f7ff 悬浮背景色 + --tv-color-bg-hover-carousel-arrow: var(--tv-base-color-common-5); // #e6e6e6 轮播箭头背景悬浮色 + --tv-color-bg-hover-general: var(--tv-base-color-brand-1); // #f0f7ff 悬浮背景色This change would make the purpose of each variable more immediately clear to developers using the design system.
packages/theme/src/button-group/vars.less (1)
13-75: Consider translating comments to English for consistencyThe comments throughout the file are in Chinese. To maintain consistency and ensure that all team members can easily understand and maintain the code, consider translating the comments into English.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- examples/sites/demos/apis/button-group.js (0 hunks)
- examples/sites/demos/pc/app/button-group/basic-usage.spec.ts (1 hunks)
- examples/sites/demos/pc/app/button-group/button-group-multiple.spec.ts (1 hunks)
- examples/sites/demos/pc/app/button-group/disabled-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/button-group/disabled.spec.ts (1 hunks)
- examples/sites/demos/pc/app/button-group/disabled.vue (0 hunks)
- examples/sites/demos/pc/app/button-group/show-edit-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/button-group/show-edit.spec.ts (0 hunks)
- examples/sites/demos/pc/app/button-group/show-edit.vue (0 hunks)
- examples/sites/demos/pc/app/button-group/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/button-group/size.spec.ts (2 hunks)
- examples/sites/demos/pc/app/button-group/size.vue (1 hunks)
- examples/sites/demos/pc/app/button-group/slot-default.spec.ts (1 hunks)
- examples/sites/demos/pc/app/button-group/slot-empty.spec.ts (1 hunks)
- examples/sites/demos/pc/app/button-group/webdoc/button-group.js (0 hunks)
- packages/theme/src/base/vars.less (1 hunks)
- packages/theme/src/button-group/index.less (1 hunks)
- packages/theme/src/button-group/vars.less (1 hunks)
- packages/theme/src/carousel/vars.less (1 hunks)
- packages/vue/src/button-group/src/pc.vue (1 hunks)
💤 Files with no reviewable changes (7)
- examples/sites/demos/apis/button-group.js
- examples/sites/demos/pc/app/button-group/disabled-composition-api.vue
- examples/sites/demos/pc/app/button-group/disabled.vue
- examples/sites/demos/pc/app/button-group/show-edit-composition-api.vue
- examples/sites/demos/pc/app/button-group/show-edit.spec.ts
- examples/sites/demos/pc/app/button-group/show-edit.vue
- examples/sites/demos/pc/app/button-group/webdoc/button-group.js
🧰 Additional context used
🔇 Additional comments (15)
examples/sites/demos/pc/app/button-group/slot-empty.spec.ts (1)
9-9: Verify the color change for the empty button group element.The expected color for the first empty button group element has been updated from 'rgb(173, 176, 184)' to 'rgb(128, 128, 128)'. This change aligns with the PR objectives of refactoring theme variables for the button-group component.
Please confirm that this color change is intentional and consistent with the updated theme variables. Additionally, run the following script to check for consistency across the component:
This script will help ensure that the color change has been applied consistently across the codebase and that no instances of the old color value remain.
examples/sites/demos/pc/app/button-group/size-composition-api.vue (3)
Line range hint
10-21: Script section looks good!The script section effectively uses Vue 3 composition API with the setup syntax. The reactive references for
checkedValandgroupDataare correctly defined and align with their usage in the template.
Line range hint
23-28: Style section is appropriate for the demo.The scoped style section provides minimal but effective styling for the button groups. The margin and display properties ensure proper spacing and layout of the button groups in the demo.
Line range hint
1-28: Overall, excellent enhancement to the button-group size demonstration!This change effectively improves the button-group size demonstration by adding a default-sized group. The component maintains a clean structure, follows Vue 3 best practices, and provides a comprehensive showcase of button group sizes.
Great job on this focused and valuable enhancement to the demo!
examples/sites/demos/pc/app/button-group/basic-usage.spec.ts (1)
Line range hint
1-22: Verify test coverage and potential missing changes.The AI summary mentioned the removal of a
border-bottom-colorassertion, which is not visible in the provided code. Please verify if this assertion was indeed removed and if so, ensure that the test coverage is still adequate without it.To check for any recent changes to this file that might not be visible in the current diff, run the following command:
#!/bin/bash # Description: Check recent changes to the file git log -p -- examples/sites/demos/pc/app/button-group/basic-usage.spec.ts | head -n 50This will show the most recent changes to the file, which can help identify if any assertions were removed in a previous commit.
examples/sites/demos/pc/app/button-group/button-group-multiple.spec.ts (1)
16-16: 🛠️ Refactor suggestionVerify the new
bottomSpacevalue and update related documentation.The
bottomSpaceconstant has been changed from 4 to 2. This adjustment affects the expected height calculations for both two-row and three-row scenarios in the test cases. While the test logic remains valid, this change might indicate a visual adjustment in the component's styling.
- Please confirm that this new value of 2 is correct and aligns with the intended styling of the button-group component.
- If this change is part of a broader theme refactoring, ensure that all related components and tests are updated consistently.
- Update any relevant documentation (e.g., design specs, component API docs) to reflect this change in spacing.
To ensure consistency across the codebase, run the following script to check for other occurrences of
bottomSpace:Consider extracting this
bottomSpacevalue into a shared configuration file for easier maintenance and consistency across components and tests.examples/sites/demos/pc/app/button-group/disabled.spec.ts (4)
21-21: LGTM! Verify accessibility of the new color.The background color change for the first button in
buttonGroup3(from rgb(94, 124, 224) to rgb(20, 118, 255)) is consistent with the theme variable refactoring. The new color is a brighter shade of blue.Please verify that the new color (rgb(20, 118, 255)) meets accessibility standards for color contrast when used with white text. You can use a tool like the WebAIM Contrast Checker to ensure compliance with WCAG 2.0 guidelines.
24-25: LGTM! Ensure consistency across button groups.The CSS property changes for the second button in
buttonGroup3(background: rgb(240, 240, 240), text: rgb(194, 194, 194)) are consistent with the theme variable refactoring and previous changes to disabled button styles.Please verify that these disabled button styles are consistently applied across all button groups in the component by running the following script:
#!/bin/bash # Description: Check for consistent disabled button styles across button groups # Test: Search for disabled button style applications in the component rg --type vue 'disabled.*class' examples/sites/demos/pc/app/button-group/
16-17: LGTM! Verify consistency of disabled button styles.The CSS property changes for the second button in
buttonGroup1are consistent with the theme variable refactoring. The new colors (background: rgb(240, 240, 240), text: rgb(194, 194, 194)) are appropriate for a disabled button state.Please verify the consistency of disabled button styles across the component by running the following script:
#!/bin/bash # Description: Check for consistent use of disabled button styles # Test: Search for disabled button style definitions rg --type less 'disabled.*\{' examples/sites/demos/pc/app/button-group/
9-9: Verify the updated button group structure.The locator for
buttonGroup3has been updated to select the second.tiny-button-groupinstead of the third. This change is consistent with the removal of the second button group mentioned in the summary.Please run the following script to verify the updated structure of the button groups in the component:
✅ Verification successful
Button group structure verified.
The number of
<tiny-button-group>instances indisabled.vuehas been reduced as expected, confirming the removal of the second button group.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of button groups in the component template # Test: Search for button group elements in the component template rg --type vue '<tiny-button-group' examples/sites/demos/pc/app/button-group/Length of output: 218
Script:
#!/bin/bash # Description: Recursively search for <tiny-button-group in all .vue files to verify the number of button groups rg --type vue '<tiny-button-group' .Length of output: 66
Script:
#!/bin/bash # Description: Recursively search for <tiny-button-group in all .vue files to verify the number of button groups rg '<tiny-button-group' . --glob '*.vue'Length of output: 9140
Script:
#!/bin/bash # Description: Count the number of <tiny-button-group> occurrences in disabled.vue rg '<tiny-button-group' examples/sites/demos/pc/app/button-group/disabled.vue | wc -lLength of output: 89
packages/theme/src/base/vars.less (3)
327-328: New hover background color variables addedTwo new hover background color variables have been introduced:
--tv-color-bg-hover-2: For carousel arrow background hover color.--tv-color-bg-hover-3: For general hover background color.These additions provide more granular control over hover states in the design system, which is a good practice for maintaining consistency and flexibility.
327-328: Summary of changes and recommendationsThe introduction of new hover background color variables enhances the flexibility of the design system. The changes are consistent with the existing structure and are well-documented. However, consider the following recommendations:
- Evaluate the naming of
--tv-color-bg-hover-3for more clarity.- Ensure that these new variables are implemented in relevant components, particularly for the carousel arrow hover state.
- Update any existing hover background styles that could benefit from these new variables for consistency across the design system.
Overall, these changes are a positive addition to the design system, providing more granular control over hover states.
327-328: Consider updating related componentsWith the introduction of these new hover background color variables, it's important to ensure that they are properly implemented in the relevant components, such as the carousel for
--tv-color-bg-hover-2.To verify the usage of these new variables, you can run the following script:
packages/theme/src/button-group/vars.less (1)
13-75: Variable renaming and additions are consistent and well-structuredAll variables have been renamed from
--ti-to--tv-consistently, following the new naming convention. The introduction of new variables enhances the styling flexibility of the button group component.packages/theme/src/button-group/index.less (1)
129-132: 🛠️ Refactor suggestionReplace hardcoded
background-colorwith theme variableIn the
&__supclass, thebackground-coloris set using a hardcoded value#fa9841. To maintain consistency and allow for easier theming, consider replacing it with a theme variable.Apply this change:
- background-color: #fa9841; + background-color: var(--tv-ButtonGroup-sup-bg-color);Likely invalid or redundant comment.
| await expect(buttonGroup.getByRole('button', { name: 'Button1' })).toHaveCSS('background-color', 'rgb(25, 25, 25)') | ||
| await expect(buttonGroup.getByRole('button', { name: 'Button2' })).toHaveCSS('background-color', 'rgb(92, 179, 0)') |
There was a problem hiding this comment.
💡 Codebase verification
Outdated color values found in multiple files.
The shell script detected several instances of the old color values rgb(94, 124, 224) and rgb(80, 212, 171) across the codebase:
examples/sites/demos/pc/app/wizard/base-usage.spec.tsexamples/sites/demos/pc/app/wizard/vertical.spec.tsexamples/sites/demos/pc/app/time-line/status.spec.tsexamples/sites/demos/pc/app/tag-group/tag-group-effect.spec.jsexamples/sites/demos/pc/app/tabs/basic-usage.spec.tsexamples/sites/demos/pc/app/link/link-style.spec.tsexamples/sites/demos/pc/app/cascader-panel/basic-usage.spec.tsexamples/sites/demos/pc/app/checkbox/checkbox-slot.spec.tsexamples/sites/demos/pc/app/bulletin-board/active-name.spec.tsexamples/sites/demos/pc/app/bulletin-board/base.spec.tsexamples/sites/demos/pc/app/base-select/automatic-dropdown.spec.ts
These findings indicate that the color scheme updates are not consistently applied across the project. Please ensure that all instances of the old colors are updated to maintain design consistency and adhere to accessibility standards.
🔗 Analysis chain
Verify the new color scheme for buttons.
The background colors for 'Button1' and 'Button2' have been updated to rgb(25, 25, 25) and rgb(92, 179, 0) respectively. These changes appear intentional and likely reflect updates in the component's styling.
Please confirm that:
- These new colors align with the project's design system.
- The contrast ratios meet accessibility standards (WCAG 2.1 AA).
- The changes are consistent across all relevant components and themes.
You may want to run the following command to check for any other occurrences of the old color values:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for old color values in all files
rg -i 'rgb\(94,\s*124,\s*224\)|rgb\(80,\s*212,\s*171\)' --type css --type less --type scss --type js --type ts
Length of output: 141
Script:
#!/bin/bash
# Search for old color values in all files with corrected file types
rg -i 'rgb\(94,\s*124,\s*224\)|rgb\(80,\s*212,\s*171\)' --type css --type less --type sass --type js --type ts
Length of output: 3343
| @@ -12,8 +12,7 @@ test('按钮组基本示例', async ({ page }) => { | |||
| await button3.click() | |||
| await demo.getByRole('button', { name: 'Button2' }).hover() | |||
| await expect(buttonGroup.locator('li').nth(2)).toHaveClass('active') | |||
| await expect(button3).toHaveCSS('background-color', 'rgb(94, 124, 224)') | |||
| await expect(button3).toHaveCSS('border-bottom-color', 'rgb(94, 124, 224)') | |||
| await expect(button3).toHaveCSS('background-color', 'rgb(20, 118, 255)') | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Color Value Usage Detected
The background color for Button3 has been updated to rgb(20, 118, 255) in examples/sites/demos/pc/app/button-group/basic-usage.spec.ts. However, the old color value rgb(94, 124, 224) is still present in multiple locations across the codebase, which may lead to inconsistencies in the UI.
Affected Files with Old Color (rgb(94, 124, 224)):
examples/sites/demos/pc/app/wizard/base-usage.spec.tsexamples/sites/demos/pc/app/wizard/vertical.spec.tsexamples/sites/demos/pc/app/time-line/status.spec.tsexamples/sites/demos/pc/app/tabs/basic-usage.spec.tsexamples/sites/demos/pc/app/tag-group/tag-group-effect.spec.jsexamples/sites/demos/pc/app/base-select/automatic-dropdown.spec.tsexamples/sites/demos/pc/app/link/link-style.spec.tsexamples/sites/demos/pc/app/checkbox/checkbox-slot.spec.tsexamples/sites/demos/pc/app/cascader-panel/basic-usage.spec.tsexamples/sites/demos/pc/app/bulletin-board/base.spec.tsexamples/sites/demos/pc/app/bulletin-board/active-name.spec.tspackages/theme/src/theme/devui-theme-map.js
Affected Files with New Color (rgb(20, 118, 255)):
examples/sites/demos/pc/app/switch/basic-usage.spec.tsexamples/sites/demos/pc/app/radio/basic-usage.spec.tsexamples/sites/demos/pc/app/checkbox/checkbox-button.spec.tsexamples/sites/demos/pc/app/checkbox/basic-usage.spec.tsexamples/sites/demos/pc/app/button-group/disabled.spec.tsexamples/sites/demos/pc/app/button-group/basic-usage.spec.tsexamples/sites/demos/pc/app/button/ghost.spec.tsexamples/sites/demos/pc/app/button/text.spec.tsexamples/sites/demos/pc/app/badge/type.spec.tsexamples/sites/demos/mobile/app/alert/basic-usage.spec.ts
Please verify whether the remaining instances of the old color should also be updated to ensure a consistent theme throughout the project.
🔗 Analysis chain
Verify the updated background color value.
The expected background color for Button3 has been changed from rgb(94, 124, 224) to rgb(20, 118, 255). This change likely reflects an update in the component's design or theme.
To ensure this change is consistent across the codebase, please run the following script:
This will help identify if the color change has been consistently applied across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of the old and new color values
echo "Searching for old color value (94, 124, 224):"
rg --type css --type less --type scss --type js --type ts "94,\s*124,\s*224" || echo "No occurrences found."
echo -e "\nSearching for new color value (20, 118, 255):"
rg --type css --type less --type scss --type js --type ts "20,\s*118,\s*255" || echo "No occurrences found."
Length of output: 2916
Script:
#!/bin/bash
# Description: Corrected script to check for other occurrences of the old and new color values
echo "Searching for old color value (94, 124, 224):"
rg --type css --type less --type js --type ts "94,\s*124,\s*224" || echo "No occurrences found."
echo -e "\nSearching for new color value (20, 118, 255):"
rg --type css --type less --type js --type ts "20,\s*118,\s*255" || echo "No occurrences found."
Length of output: 4845
| // 朴素按钮非选中项悬浮/选中文本色 | ||
| --tv-ButtonGroup-item-btn-plain-text-color: var(--tv-color-text-active); | ||
| // 按钮组圆角 | ||
| --tv-ButtonGroup-plain-border-color: var(--tv-color-border); |
There was a problem hiding this comment.
Correct the mismatched comment for --tv-ButtonGroup-plain-border-color
The comment "// 按钮组圆角" does not match the variable --tv-ButtonGroup-plain-border-color. It appears to be a copy-paste error from line 15. Please update the comment to accurately describe this variable, which relates to the border color of plain buttons.
|
|
||
| &__sup-text { | ||
| background-color: #fa9841; | ||
| padding: 0 4px; | ||
| border-top-left-radius: 4px; | ||
| border-bottom-right-radius: 4px; | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded styles with variables in &__sup-text
In the &__sup-text class, hardcoded values are used for background-color, padding, and border-radius. Consider using theme variables for these properties.
Apply this change:
- background-color: #fa9841;
- padding: 0 4px;
- border-top-left-radius: 4px;
- border-bottom-right-radius: 4px;
+ background-color: var(--tv-ButtonGroup-sup-text-bg-color);
+ padding: var(--tv-ButtonGroup-sup-text-padding);
+ border-top-left-radius: var(--tv-ButtonGroup-sup-text-border-radius);
+ border-bottom-right-radius: var(--tv-ButtonGroup-sup-text-border-radius);📝 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.
| &__sup-text { | |
| background-color: #fa9841; | |
| padding: 0 4px; | |
| border-top-left-radius: 4px; | |
| border-bottom-right-radius: 4px; | |
| } | |
| &__sup-text { | |
| background-color: var(--tv-ButtonGroup-sup-text-bg-color); | |
| padding: var(--tv-ButtonGroup-sup-text-padding); | |
| border-top-left-radius: var(--tv-ButtonGroup-sup-text-border-radius); | |
| border-bottom-right-radius: var(--tv-ButtonGroup-sup-text-border-radius); | |
| } |
|
|
||
| &__sup-icon { | ||
| background-color: #fa9841; | ||
| border-radius: 2px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme variables instead of hardcoded colors in &__sup-icon
The &__sup-icon class currently uses hardcoded color values for background-color and fill. Replace these with theme variables to ensure consistency across the theme.
Apply this change:
- background-color: #fa9841;
- fill: #fff;
+ background-color: var(--tv-ButtonGroup-sup-icon-bg-color);
+ fill: var(--tv-ButtonGroup-sup-icon-fill-color);Committable suggestion was skipped due to low confidence.
| margin-right: 2px; | ||
| margin-bottom: 2px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme variables for margin values
The &:not(:last-child) selector uses hardcoded margin-right and margin-bottom values. Replacing these with theme variables enhances maintainability and consistency.
Apply this change:
- margin-right: 2px;
- margin-bottom: 2px;
+ margin-right: var(--tv-ButtonGroup-item-margin-right);
+ margin-bottom: var(--tv-ButtonGroup-item-margin-bottom);📝 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.
| margin-right: 2px; | |
| margin-bottom: 2px; | |
| margin-right: var(--tv-ButtonGroup-item-margin-right); | |
| margin-bottom: var(--tv-ButtonGroup-item-margin-bottom); |
| &.tiny-popper { | ||
| padding: 6px 0; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of .tiny-popper class detected
The .tiny-popper class is used both as a standalone modifier and in combination with other BEM classes, which may lead to inconsistencies in naming conventions. It's recommended to align its usage with the project's BEM standards, potentially by adopting a standardized modifier approach.
🔗 Analysis chain
Verify the use of .tiny-popper class for consistency
The use of the .tiny-popper class may not align with the naming conventions used elsewhere in the stylesheet. Ensure that this class name is consistent with the project's BEM conventions and consider using a variable if appropriate.
Run the following script to check for consistent usage of the .tiny-popper class:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the usage of '.tiny-popper' class across LESS files
# Test: Find all occurrences of '.tiny-popper' to verify naming consistency
rg --type less --no-filename '\.tiny-popper'
Length of output: 144
Script:
#!/bin/bash
# Description: Extract definitions of '.tiny-popper' to assess BEM compliance
# Find all LESS files containing '.tiny-popper' and display surrounding class definitions
rg --type less '\.tiny-popper' -C 2
Length of output: 1043
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?
删除了无用的border属性示例和editor编辑示例
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
tiny-button-groupcomponent in the size demo.Bug Fixes
button-groupcomponent.Refactor
Chores