refactor(badge): [badge] refactor badge theme vars #2229
Conversation
WalkthroughThe changes in this pull request involve updates to the styling and testing of the badge component. The 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/badge/vars.less (1)
14-14: Consider translating comments to English for consistencyThe code comments are currently in Chinese. To maintain consistency and improve accessibility for all contributors, please consider translating the comments to English.
Also applies to: 16-16, 18-18, 20-20, 22-22, 24-24, 26-26, 28-28, 30-30, 32-32, 34-34, 36-36, 38-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/sites/demos/pc/app/badge/type.spec.ts (1 hunks)
- packages/theme/src/badge/index.less (3 hunks)
- packages/theme/src/badge/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
examples/sites/demos/pc/app/badge/type.spec.ts (1)
24-24: Approve change, verify new color value, and check accessibilityThe updated background color for the info badge (rgb(20, 118, 255)) is consistent with the PR's refactoring objectives. This change represents a significant shift from the previous dark gray to a bright blue.
Please ensure the following:
- Confirm that this new color value aligns with the updated design system and is consistently applied across the application.
- Verify that the contrast between this new background color and the text color meets accessibility standards (WCAG 2.1 AA).
You may want to run the following command to check for any other occurrences of the old color value:
rg --type less --type typescript "rgb\(25, 25, 25\)"Additionally, consider using a color contrast checker tool to ensure the new color combination meets accessibility standards.
packages/theme/src/badge/vars.less (1)
13-39: Variables are well-defined and follow naming conventionsThe refactored badge theme variables are properly defined and align with the project's naming conventions, improving code maintainability.
packages/theme/src/badge/index.less (4)
58-58: Confirm the definition of--tv-Badge-dot-bg-color.The background color for
&--defaultuses--tv-Badge-dot-bg-color. Ensure this variable is defined and holds the correct value for the default badge dot.
67-83: Verify the new CSS variables for badge types are defined.The background colors for badge modifiers (
&--primary,&--success,&--warning,&--danger,&--info) now use new variables prefixed with--tv-Badge-. Please ensure all these variables are defined invars.lessand have appropriate values.You can run the following script to check the definitions:
#!/bin/bash # Description: Verify that all new badge background color variables are defined. # List of new variables VARIABLES=( '--tv-Badge-primary-bg-color' '--tv-Badge-success-bg-color' '--tv-Badge-warning-bg-color' '--tv-Badge-danger-bg-color' '--tv-Badge-info-bg-color' ) for var in "${VARIABLES[@]}"; do echo "Checking for $var in vars.less" rg --type less "$var" packages/theme/src/badge/vars.less || echo "Variable $var is missing!" doneWould you like assistance in defining any missing variables?
49-51: Ensure anchor tag styles are consistently applied.The color for
aanda:hoveris set tovar(--tv-Badge-a-text-color). Verify that this variable is defined and that it provides appropriate contrast and accessibility.You can run the following script to confirm the variable definition:
#!/bin/bash # Description: Verify that `--tv-Badge-a-text-color` is defined. # Test: Search for the variable definition. Expect: At least one occurrence in Less files. rg --type less '\-\-tv\-Badge\-a\-text\-color' -A 5
20-20: Verify the definition and import of.inject-Badge-vars().The mixin
.inject-Badge-vars();is being used. Please ensure that this mixin is properly defined and imported in the codebase to prevent any compilation errors.You can run the following script to verify if
.inject-Badge-vars()is defined:✅ Verification successful
Verified: The
.inject-Badge-vars()mixin is properly defined inpackages/theme/src/badge/vars.lessand correctly imported inpackages/theme/src/badge/index.less.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of the `.inject-Badge-vars()` mixin. # Test: Search for the mixin definition. Expect: At least one occurrence in Less files. rg --type less --pcre2 '^\.inject-Badge-vars\(\)' -A 5Length of output: 3960
Script:
#!/bin/bash # Description: Verify the definition of the `.inject-Badge-vars()` mixin. # Search for the mixin definition in Less files rg --type less '\.inject-Badge-vars\s*\(\s*\)' # Search for import statements related to the mixin in Less files rg --type less '@import.*vars.less' packages/theme/src/badge/index.lessLength of output: 265
| @@ -13,13 +13,13 @@ test('主題样式', async ({ page }) => { | |||
| const primaryBadge = badge.nth(5) | |||
|
|
|||
| await expect(dangerBadge).toHaveClass(/tiny-badge--danger/) | |||
| await expect(dangerBadge).toHaveCSS('background-color', 'rgb(246, 111, 106)') | |||
| await expect(dangerBadge).toHaveCSS('background-color', 'rgb(242, 48, 48)') | |||
There was a problem hiding this comment.
💡 Codebase verification
Old color value 'rgb(246, 111, 106)' still present in other test files
The color rgb(246, 111, 106) is still used in the following files:
examples/sites/demos/pc/app/time-line/status.spec.ts
Please update these instances to align with the new design system.
🔗 Analysis chain
Approve change and verify new color value
The updated background color for the danger badge (rgb(242, 48, 48)) is consistent with the PR's refactoring objectives. This change appears to introduce a more vibrant red color.
Please confirm that this new color value aligns with the updated design system and is consistently applied across the application. You may want to run the following command to check for any other occurrences of the old color value:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type less --type typescript "rgb\(246, 111, 106\)"
Length of output: 229
Script:
#!/bin/bash
# Searching for the old dangerBadge color value in TypeScript files
rg -g "*.ts" "rgb\\(246, 111, 106\\)"
Length of output: 354
| min-width: 16px; | ||
| height: 16px; | ||
| line-height: 16px; | ||
| border-radius: var(--tv-Badge-border-radius); | ||
| padding: 0 var(--tv-Badge-horizontal-padding); | ||
| font-size: var(--tv-Badge-font-size); | ||
| font-weight: var(--tv-Badge-font-weight); | ||
| color: var(--tv-Badge-text-color); | ||
| background-color: var(--tv-Badge-bg-color); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using relative units for better responsiveness.
The use of fixed pixel values for min-width, height, and line-height may affect responsiveness on different screen sizes. Consider using relative units like em or rem to enhance scalability across various devices.
| vertical-align: baseline; | ||
| white-space: nowrap; | ||
| text-align: center; | ||
| margin-left: var(--ti-badge-margin-left); | ||
| border: var(--ti-badge-border) solid #FFFFFF; | ||
| border: 1px solid #FFFFFF; |
There was a problem hiding this comment.
Replace hardcoded border color with a CSS variable for consistency.
The border color is hardcoded as #FFFFFF. To maintain consistency and facilitate theming, it's recommended to use a CSS variable instead.
Apply this diff to define and use a border color variable:
+ // Define the variable in vars.less
+ --tv-Badge-border-color: #FFFFFF;
// In index.less
- border: 1px solid #FFFFFF;
+ border: 1px solid var(--tv-Badge-border-color);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 (8)
examples/sites/demos/pc/app/link/basic-usage.vue (1)
17-21: Consider the impact of increased margin on layout.The
margin-rightfor.tiny-linkhas been significantly increased from 8px to 48px. While this change aligns with the AI summary, it's worth considering the following:
- This large margin might affect the overall layout, especially on smaller screens.
- Ensure this change is intentional and consistent with the design guidelines.
- Consider using a responsive approach or a design token for this spacing to maintain consistency across the application.
Would you like assistance in implementing a more flexible spacing solution?
examples/sites/demos/pc/app/link/dynamic-disable-composition-api.vue (1)
16-19: Consider the impact of increased margin on responsive layouts.The margin-right for .tiny-link has been significantly increased from 8px to 48px. While this provides more spacing between links, it may cause layout issues on smaller screens or when many links are present.
Consider the following suggestions:
- Verify that this change doesn't negatively impact the layout on various screen sizes.
- Consider using a responsive approach, such as:
.tiny-link { margin-right: 16px; } @media (min-width: 768px) { .tiny-link { margin-right: 48px; } }This approach would provide a smaller margin on mobile devices and the larger margin on larger screens.
examples/sites/demos/pc/app/link/dynamic-disable.vue (1)
22-25: Consider explaining the rationale for the increased margin.The right margin for
.tiny-linkhas been significantly increased from 8px to 48px. While this will create more space between links and potentially improve readability, it might also lead to excessive whitespace.Could you provide a rationale for this change? If the goal is to improve spacing, consider whether a smaller increase (e.g., to 16px or 24px) might achieve the desired effect without creating too much separation between the links.
examples/sites/demos/pc/app/link/size-composition-api.vue (2)
Line range hint
1-9: Consider using CSS for layout control instead of
tags.The template structure is clean and demonstrates various uses of the TinyLink component. However, using
tags for spacing is not a recommended practice in modern web development.Consider using CSS flexbox or grid for better layout control. You could wrap the links in a container and use CSS to manage spacing:
<template> - <div class="tiny-custom"> - <tiny-link :underline="false">默认链接</tiny-link> - <br /> - <tiny-link :underline="false" size="medium">默认链接</tiny-link> - <br /> - <tiny-link :underline="false" :icon="TinyIconEdit">编辑</tiny-link> - <br /> - <tiny-link :underline="false" size="medium" :icon="TinyIconEdit">编辑</tiny-link> + <div class="tiny-custom link-container"> + <tiny-link :underline="false">默认链接</tiny-link> + <tiny-link :underline="false" size="medium">默认链接</tiny-link> + <tiny-link :underline="false" :icon="TinyIconEdit">编辑</tiny-link> + <tiny-link :underline="false" size="medium" :icon="TinyIconEdit">编辑</tiny-link> </div> </template>Then add appropriate CSS for spacing in the style section.
Line range hint
18-33: Style changes look good, consider using CSS variables for consistency.The addition of margin-right to .tiny-link is a good way to add spacing between links. This aligns well with the earlier suggestion to remove
tags from the template.For better maintainability and consistency across the application, consider using CSS variables for spacing values. For example:
<style scoped> +:root { + --tiny-link-spacing: 48px; +} .tiny-custom { display: flex; align-items: flex-start; justify-content: space-between; flex-direction: column; } .tiny-custom-view, .tiny-custom-del { margin-left: 2px; } .tiny-link { - margin-right: 48px; + margin-right: var(--tiny-link-spacing); } </style>This approach allows for easier global updates to spacing in the future.
examples/sites/demos/pc/app/link/size.vue (1)
Line range hint
1-11: LGTM! Consider using CSS for spacing instead of<br />tags.The changes to the template look good. The addition of
sizeandiconprops demonstrates different variations of thetiny-linkcomponent. However, using<br />tags for spacing is not ideal in modern web development.Consider using CSS flexbox or grid for layout and spacing instead of
<br />tags. This would make the layout more flexible and easier to maintain. For example:<template> <div class="tiny-custom"> <tiny-link :underline="false">默认链接</tiny-link> <tiny-link :underline="false" size="medium">默认链接</tiny-link> <tiny-link :underline="false" :icon="TinyIconEdit">编辑</tiny-link> <tiny-link :underline="false" size="medium" :icon="TinyIconEdit">编辑</tiny-link> </div> </template> <style scoped> .tiny-custom { display: flex; flex-direction: column; gap: 10px; /* Adjust the value to control spacing between links */ } </style>examples/sites/demos/pc/app/link/custom-icon-composition-api.vue (1)
Line range hint
1-18: Consider standardizing icon implementation across TinyLink components.The template demonstrates various ways of implementing icons within TinyLink components, which showcases flexibility but might lead to inconsistency. Consider standardizing the approach across all instances for better maintainability.
For example, you could consistently use the
:iconprop approach:<tiny-link :icon="TinyIconFilletExternalLink">链接按钮</tiny-link>Or consistently use the slot-based approach:
<tiny-link> <template #icon> <tiny-icon-fillet-external-link class="tiny-svg-size"></tiny-icon-fillet-external-link> </template> 链接按钮 </tiny-link>Choosing one consistent method could improve code readability and maintenance.
examples/sites/demos/pc/app/link/custom-icon.vue (1)
Line range hint
1-17: Consider using a consistent approach for icon placementThe template uses two different methods for icon placement: slot-based (
<template #icon>) and property-based (:icon="IconEdit"). While both are valid, using a consistent approach throughout the component would improve readability and maintainability.Consider using the slot-based approach for all icons:
<tiny-link> <template #icon><icon-fillet-external-link class="tiny-svg-link"></icon-fillet-external-link></template> 链接按钮 </tiny-link>Or use the property-based approach consistently:
<tiny-link :icon="IconFilletExternalLink">链接按钮</tiny-link>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- examples/sites/demos/pc/app/link/basic-usage-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/link/basic-usage.vue (1 hunks)
- examples/sites/demos/pc/app/link/custom-icon-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/link/custom-icon.vue (1 hunks)
- examples/sites/demos/pc/app/link/dynamic-disable-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/link/dynamic-disable.vue (1 hunks)
- examples/sites/demos/pc/app/link/focus-no-underline-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/link/focus-no-underline.vue (1 hunks)
- examples/sites/demos/pc/app/link/link-style-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/link/link-style.vue (1 hunks)
- examples/sites/demos/pc/app/link/size-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/link/size.vue (1 hunks)
🧰 Additional context used
🔇 Additional comments (17)
examples/sites/demos/pc/app/link/basic-usage-composition-api.vue (1)
14-14: 🛠️ Refactor suggestionVerify the increased margin between links
The margin-right for .tiny-link has been significantly increased to 48px. While this provides more spacing between links, it might have unintended consequences on the layout, especially on smaller screens or when multiple links are displayed in a row.
Please confirm that this change:
- Aligns with the intended design across various screen sizes.
- Doesn't cause layout issues when multiple links are present.
- Is consistent with other components and the overall UI design.
Consider using a responsive approach if needed, such as:
.tiny-link { margin-right: 24px; /* Default for smaller screens */ } @media (min-width: 768px) { .tiny-link { margin-right: 48px; /* Larger margin for bigger screens */ } }To verify the visual impact, you can run the following commands:
This will help ensure consistency across the application and identify any potential conflicts with the new margin.
examples/sites/demos/pc/app/link/focus-no-underline-composition-api.vue (1)
14-14: Approve change, but verify layout impactThe margin-right for .tiny-link has been increased from 8px to 48px. This change is valid and will provide more spacing between link components. However, it's a significant increase that may impact the overall layout of the demo.
Please ensure this change aligns with the intended design and doesn't negatively affect the layout of other demos or the component's documentation page. You may want to visually inspect the changes in the browser to confirm the desired appearance.
examples/sites/demos/pc/app/link/basic-usage.vue (2)
Line range hint
1-6: LGTM: Template structure is correct and consistent.The template section correctly uses two
tiny-linkcomponents, demonstrating different ways of setting the link text (via content andvalueprop). This structure aligns with the AI summary and provides a good example of basic usage.
Line range hint
8-15: LGTM: Component import and registration are correct.The script section correctly imports the
Linkcomponent from '@opentiny/vue' and registers it asTinyLink. This aligns with the AI summary and follows Vue.js best practices for component registration.examples/sites/demos/pc/app/link/focus-no-underline.vue (1)
20-20: Increased spacing between link components.The margin-right for .tiny-link has been significantly increased from 8px to 48px. This change aligns with the PR's refactoring objectives and provides more breathing room between link components.
Please verify that this increased spacing doesn't negatively impact the overall layout or responsiveness of the component, especially in constrained viewport sizes. Consider testing the component in various screen sizes to ensure optimal display.
✅ Verification successful
Increased spacing between link components.
The margin-right for
.tiny-linkhas been significantly increased from 8px to 48px. Automated checks did not reveal any responsive design elements infocus-no-underline.vuethat could be impacted by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any responsive design considerations in the codebase # Test: Search for responsive design related code rg --type vue -i '(media|@media|responsive|screen size|viewport)'Length of output: 95
Script:
#!/bin/bash # Description: Check for any responsive design considerations in .vue files # Test: Search for responsive design related code within .vue files rg -g "*.vue" -i '(media|@media|responsive|screen size|viewport)'Length of output: 4215
examples/sites/demos/pc/app/link/link-style-composition-api.vue (1)
18-18: Increased spacing between links: Please clarify the rationale.The margin-right for .tiny-link has been significantly increased from 8px to 48px. While this change can improve readability and visual separation between links, it's a substantial increase that might affect the overall layout.
- Could you please provide the reasoning behind choosing 48px specifically?
- Has this change been tested on various screen sizes to ensure it doesn't cause layout issues on smaller devices?
- Is this part of a broader design update, or an isolated change?
Consider using a responsive approach, such as using rem units or media queries, to ensure the spacing works well across different screen sizes.
examples/sites/demos/pc/app/link/link-style.vue (1)
22-25: Increased margin between links may affect layout.The change to increase the
margin-rightof.tiny-linkfrom 8px to 48px is noted. This will provide more spacing between links, potentially improving readability.Consider the impact of this change on the overall layout, especially in responsive designs or constrained spaces. You may want to test this change across different screen sizes to ensure it doesn't cause unintended layout issues.
examples/sites/demos/pc/app/link/dynamic-disable-composition-api.vue (2)
Line range hint
1-9: LGTM: Template structure is well-organized and demonstrates various link types.The template section effectively showcases different types of disabled links using the
<tiny-link>component. The consistent use of thedisabledattribute aligns with the file's purpose of demonstrating dynamic disabling.
Line range hint
11-13: LGTM: Script setup is concise and follows best practices.The script section correctly imports the Link component from '@opentiny/vue' and aliases it as TinyLink. The use of the Composition API with the
setupsyntax is appropriate for this component.examples/sites/demos/pc/app/link/dynamic-disable.vue (2)
Line range hint
1-10: LGTM: Template section is well-structured and demonstrates various link types.The template effectively showcases different
<tiny-link>types (default, primary, success, warning, danger, info) in a disabled state. This aligns well with the component's purpose of demonstrating dynamically disabled links.
Line range hint
12-19: LGTM: Script section correctly imports and registers the Link component.The
Linkcomponent is properly imported from '@opentiny/vue' and registered as 'TinyLink'. This setup correctly supports the usage in the template section.examples/sites/demos/pc/app/link/size-composition-api.vue (1)
Line range hint
11-16: LGTM! Script setup looks good.The script section correctly imports the necessary components and icons, and follows Vue 3 composition API best practices. The use of 'as' for renaming the Link component to TinyLink maintains consistency with the template usage.
examples/sites/demos/pc/app/link/size.vue (1)
Line range hint
13-25: LGTM! Correct implementation of icon import and usage.The changes in the script section are correct and necessary for using the icon in the template. The
TinyIconEditis properly added to thedata()function, and theiconEditis correctly imported from '@opentiny/vue-icon'.examples/sites/demos/pc/app/link/custom-icon-composition-api.vue (2)
Line range hint
20-28: LGTM! Well-structured script section.The script section demonstrates good practices:
- Utilizes Vue 3 Composition API with the
setupsyntax.- Clearly imports necessary components and icons.
- Consistently defines constants for icon components.
This approach enhances readability and maintainability of the component.
Line range hint
30-35: Verify the significant increase in margin-right.The margin-right for .tiny-link has been increased from 8px to 48px. This is a substantial change that will significantly affect the spacing between links.
.tiny-link { - margin-right: 8px; + margin-right: 48px; }Please confirm if this increase is intentional and aligns with the desired layout. If it's meant to improve readability or match a specific design requirement, it might be worth adding a comment explaining the rationale behind this change.
examples/sites/demos/pc/app/link/custom-icon.vue (2)
Line range hint
19-35: LGTM: Script section changes look goodThe script section correctly imports and sets up the necessary components and icons. The use of function calls for icon components (e.g.,
IconView()) is consistent with Vue 3 syntax, and theIconEditdata property is correctly initialized.
39-43: Verify the increased margin-right for .tiny-linkThe margin-right for .tiny-link has been significantly increased from 8px to 48px. This large change might affect the layout and spacing of the links more than intended.
Please confirm if this change is intentional and verify its impact on the component's appearance. Consider using a design system variable for consistency if available.
✅ Verification successful
Margin-right change for
.tiny-linkis consistentThe
margin-rightfor.tiny-linkhas been increased consistently across all link components from 8px to 48px. This change appears intentional and aligns with the design requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of margin-right in link components # Expected result: No other conflicting margin-right values for link components echo "Searching for other margin-right declarations in link components:" rg --type vue 'margin-right' -C 2 | grep -i 'link'Length of output: 217
Script:
#!/bin/bash # Description: Check for other occurrences of margin-right in link components within .vue files # Expected result: No other conflicting margin-right values for link components echo "Searching for other margin-right declarations in link components within .vue files:" rg 'margin-right' --glob '*.vue' -C 2 | grep -i 'link'Length of output: 4385
| .tiny-link { | ||
| margin-right: 48px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adjusting the margin for better flexibility.
The addition of margin-right: 48px; to .tiny-link improves spacing between link components. However, this fixed margin might not be ideal for all screen sizes and could potentially conflict with the existing flex layout.
Consider using a more flexible approach:
- Use
gapproperty in the flex container instead of individual margins:
.tiny-custom {
display: flex;
flex-direction: column;
gap: 16px; /* Adjust as needed */
}- If you need horizontal spacing for inline usage, consider using CSS custom properties for easier maintenance:
.tiny-link {
margin-right: var(--tiny-link-margin, 16px);
}This allows for easy customization and responsiveness by adjusting the --tiny-link-margin variable as needed.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
tiny-linkcomponent with modified styling and structure.dangerBadgeandinfoBadgein tests to ensure accurate rendering.