feat(card): Adapting to the SMB themes #2231
Conversation
WalkthroughThe changes in this pull request involve a comprehensive update to the card component's styling in the Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/theme/src/card/vars.less (3)
36-48: Improved state-based styling with design system integration.The border color variables for different card states have been updated to follow the new naming convention and now reference base color variables from the design system. This change improves consistency and makes it easier to maintain a cohesive look across components.
However, there's an opportunity for further improvement:
- The alerting state color (#fac20a) is hardcoded. Consider using a variable for consistency, e.g.,
var(--tv-color-act-alerting-border).- The default border color is set to #ffffff (white). Ensure this is intentional, as it might not be visible on white backgrounds.
51-57: Updated body padding variables with partial design system integration.The body padding variables for all card sizes have been updated to follow the new naming convention. The use of space variables for small and mini sizes improves consistency with the design system.
Suggestion for improvement:
Consider using space variables for large and medium sizes as well, to maintain consistency across all size variants and to better integrate with the design system. For example:--tv-Card-large-body-padding: var(--tv-space-3xl, 32px); --tv-Card-medium-body-padding: var(--tv-space-2xl, 24px);This change would make it easier to maintain and adjust padding values globally.
Line range hint
1-81: Overall improvement in design system integration with minor suggestions for further enhancement.The changes to the card component variables demonstrate a significant improvement in aligning with the design system. The consistent naming convention and extensive use of design system variables enhance maintainability and ensure visual coherence across the application.
Summary of improvements:
- Consistent naming convention (
--tv-Card-*) across all variables.- Extensive use of design system variables for colors, typography, spacing, and sizing.
- Improved flexibility for future design system updates.
Suggestions for further enhancement:
- Replace the hardcoded alerting color (#fac20a) with a design system variable.
- Consider using spacing variables for large and medium card body padding.
- Review the default border color (white) to ensure it's intentional and visible on all backgrounds.
These minor adjustments would further improve the consistency and maintainability of the card component within the design system.
packages/theme/src/card/index.less (2)
29-29: Consider maintaining hover effect for better UXRemoving the
box-shadowon hover eliminates visual feedback for user interaction. This might negatively impact user experience. Consider maintaining some form of hover effect, even if it's subtle, to indicate interactivity.
153-154: Consider using variables for image border radiusThe border radius for the top corners of images has been set to explicit pixel values. Consider using variables for consistency with the rest of the component, especially if these should match the card's border radius.
Suggestion:
- border-top-left-radius: 8px; - border-top-right-radius: 8px; + border-top-left-radius: var(--tv-Card-image-border-radius, 8px); + border-top-right-radius: var(--tv-Card-image-border-radius, 8px);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/card/index.less (10 hunks)
- packages/theme/src/card/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
packages/theme/src/card/vars.less (5)
15-15: Improved naming convention and value consistency.The variable has been renamed from
--ti-card-bg-colorto--tv-Card-bg-color, adopting a more consistent naming convention with PascalCase for the component name. The value now references a base variable--tv-color-bg-secondary, which improves consistency across the design system.
18-24: Consistent naming and flexible sizing for card widths.The width variables for all card sizes (large, medium, small, mini) have been updated with the new naming convention. The use of
var(--tv-size-base, 4px)as a base unit provides flexibility for future adjustments to the design system. This change improves consistency and maintainability.
27-33: Consistent border radius variables aligned with design system.The border radius variables for all card sizes have been updated to follow the new naming convention. The values now reference base border radius variables, which enhances consistency with the overall design system and allows for easier global updates.
60-68: Improved typography consistency with design system integration.The typography variables for the card component have been updated to follow the new naming convention and now reference design system variables for font sizes, colors, and weights. This change significantly improves consistency with the overall design system and makes it easier to maintain a cohesive typographic style across components.
The use of variables like
var(--tv-font-size-lg)andvar(--tv-color-act-primary-text)ensures that any global changes to the design system will be automatically reflected in the card component, improving maintainability.
71-81: Enhanced consistency in option and footer styling with design system integration.The variables for card options, disabled state, footer, and title margin have been updated to follow the new naming convention and now reference design system variables. This change improves consistency with the overall design system and enhances maintainability.
Key improvements:
- Use of semantic color variables (e.g.,
var(--tv-color-bg-primary)for option text color)- Consistent font size references (e.g.,
var(--tv-font-size-lg))- Adoption of spacing variables (e.g.,
var(--tv-space-xl)for title margin)These changes ensure that the card component will automatically reflect any global design system updates, maintaining visual consistency across the application.
packages/theme/src/card/index.less (11)
34-46: LGTM: Consistent variable updates for border radiusThe updates to the border radius variables for different card sizes are consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design.
50-62: LGTM: Consistent variable updates for card widthsThe updates to the width variables for different card sizes are consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design.
67-83: LGTM: Consistent variable updates for card statesThe updates to the border color variables for different card states (success, warning, alerting, danger, default) are consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design and functionality.
131-131: LGTM: Consistent variable update for disabled stateThe update to the background color variable for the disabled state is consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design.
169-181: LGTM: Consistent variable updates for card paddingThe updates to the padding variables for different card sizes are consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design.
212-222: LGTM: Consistent variable updates for typographyThe updates to the typography-related variables (font size, weight, margin, and color) for title and main text are consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design.
249-254: LGTM: Consistent variable updates for footer optionsThe updates to the variables for footer options (border color, text color, and font size) are consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design.
Line range hint
268-288: LGTM: Consistent variable updates for various statesThe updates to the color variables for disabled icon, footer more text, and disabled dropdown item are consistent with the new naming convention. The logic remains unchanged, which is good for maintaining the existing design and functionality.
Line range hint
1-301: Summary of Card Component Styling ChangesThe changes in this file primarily involve updating CSS variable names from
--ti-to--tv-, which appears to be part of a larger effort to standardize naming conventions. Most changes are straightforward and maintain the existing design logic. However, a few points require attention:
- The change to
min-width: automight affect layout behavior.- Removal of the hover effect's box-shadow could impact user experience.
- Changing the display property to
flexfor checkbox-checked items might affect their layout.- Explicit border radius values for images could be replaced with variables for consistency.
Please address these points and ensure thorough testing across different viewport sizes and states to maintain the component's integrity and user experience.
To ensure consistency across the entire component, consider running:
#!/bin/bash # Check for any remaining --ti- prefixed variables that might have been missed rg --type less "--ti-" packages/theme/src/card/
Line range hint
88-114: Verify the impact of changing display to flex for checked itemsWhile the border color update is consistent with the new naming convention, changing the display property to
flexfor checkbox-checked items might affect their layout. Please ensure this change doesn't cause unexpected layout shifts or alignment issues.Consider running the following test:
✅ Verification successful
Display property change verified. No layout issues detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing flex layouts or related styles that might interact with this change rg --type less "(display:\s*flex|align-items|justify-content)" packages/theme/src/card/Length of output: 647
Script:
#!/bin/bash # Search for any existing flex layouts or related styles that might interact with this change rg --type less "(display:\s*flex|align-items|justify-content)" packages/theme/src/card/Length of output: 771
22-26: Verify the impact ofmin-width: autoThe change from a specific
min-widthvalue toautomight affect the card's layout behavior, especially in responsive designs. Please ensure this change doesn't cause unexpected layout shifts or breakages across different viewport sizes.Consider running the following test:
✅ Verification successful
Further investigate usage of
min-width: autowithin media queriesPlease run the following script to identify if
min-width: autois used within any media queries, which could impact responsive design behaviors:
Correct the search command for
min-width: autowithin media queriesThe previous script encountered a syntax error. Please run the corrected script below to search for
min-width: autowithin.lessfiles inpackages/theme/src/:
Run the corrected script to search for
min-width: autowithin media queriesPlease execute the following script to accurately identify any instances of
min-width: autowithin media queries in.lessfiles:
Run the corrected script to search for
min-width: autowithin Less filesPlease execute the following script to accurately identify any instances of
min-width: autowithin.lessfiles inpackages/theme/src/:
min-widthupdated toautodoes not impact media queriesThe change to
min-width: autohas been applied consistently across the following files:
packages/theme/src/card/index.lesspackages/theme/src/transfer/index.lesspackages/theme/src/query-builder/index.lesspackages/theme/src/grid-toolbar/index.lesspackages/theme/src/badge/index.lessThis update does not affect any media queries or responsive design breakpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any responsive design breakpoints or media queries that might be affected rg --type less "(@media|@custom-media)" packages/theme/src/Length of output: 4929
Script:
#!/bin/bash # Search for 'min-width: auto' within media queries in Less files rg --type less '@media' -A5 'min-width:\s*auto' packages/theme/src/Length of output: 20273
Script:
#!/bin/bash # Search for 'min-width: auto' within media queries in Less files rg --type less '@media' packages/theme/src/ | rg 'min-width:\s*auto'Length of output: 68
Script:
#!/bin/bash # Search for 'min-width: auto' within media queries in Less files rg --type less '@media' packages/theme/src/ | rg 'min-width:\s*auto'Length of output: 68
Script:
#!/bin/bash # Search for 'min-width: auto' in Less files within the specified directory rg --type less 'min-width:\s*auto' packages/theme/src/Length of output: 366
Youyou-smiles
changed the title
|
[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 |
| @@ -12,76 +12,71 @@ | |||
|
|
|||
| .component-css-vars-amount() { | |||
There was a problem hiding this comment.
component-css-vars-amount 改成 inject-Card-vars
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
New Features
Bug Fixes
These changes aim to provide a more cohesive design experience for users interacting with card components.