feat(card): Simplification and optimization of card components #2496
Conversation
WalkthroughThe changes in this pull request involve significant modifications to 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: 3
🧹 Outside diff range and nitpick comments (6)
examples/sites/demos/pc/app/card/custom-class-composition-api.vue (1)
17-19: LGTM! Clear and focused styling.The styles effectively demonstrate custom class usage with appropriate scoping. Consider using CSS custom properties for the background color to maintain consistency with the design system.
.demo-card-custom-class { padding: 20px; - background-color: #f0f0f0; + background-color: var(--ti-base-color-bg-1, #f0f0f0); }examples/sites/demos/pc/app/card/custom-class.vue (1)
23-25: Add documentation to explain the demo's purpose.While the styling is clear, consider adding comments to explain:
- The purpose of this demo
- How users can apply custom classes to cards
- Best practices for card styling
Example improvement:
<template> + <!-- This demo shows how to apply custom styling to TinyCard components using the custom-class prop --> <div> <tiny-card title="这是卡片标题" custom-class="demo-card-custom-class">packages/theme/src/card/index.less (1)
Line range hint
1-276: Consider standardizing all border-radius values.The file contains multiple hardcoded border-radius values (8px, 6px) in different places. Consider:
- Creating a set of standardized border-radius CSS variables
- Using these variables consistently throughout the component
Example approach:
// In vars.less @card-border-radius-lg: 8px; @card-border-radius-sm: 6px; // Usage in the component --tv-Card-border-radius: @card-border-radius-lg; --tv-Card-checkbox-border-radius: @card-border-radius-sm;examples/sites/demos/pc/app/card/webdoc/card.js (1)
56-57: Improve documentation comment structure.The current comment "以下demo示例将暂不暴露" (These demo examples will not be exposed temporarily) could be more informative.
Consider replacing with a more detailed comment that explains:
- Why these features are being removed
- Whether they will be reintroduced in a different form
- What alternatives users should consider
- // 以下demo示例将暂不暴露 + // The following demos have been temporarily removed as part of the Card component simplification effort. + // These features will be reimplemented in a future release with improved APIs. + // For now, please use the basic card features and custom styling for similar functionality.examples/sites/demos/apis/card.js (2)
295-322: Document alternative patterns for operation buttonsWith the removal of the
IOptionsinterface and related functionality, please:
- Document recommended patterns for implementing operation buttons
- Consider providing examples of custom implementations in the component documentation
Line range hint
1-322: Add migration guide for breaking changesThis PR introduces significant breaking changes by removing multiple properties, all events, and type definitions. Please:
- Add a migration guide documenting:
- Removed APIs and their replacements/workarounds
- Examples of migrating common use cases
- Timeline for deprecation (if applicable)
- Consider a phased approach:
- Deprecate APIs first with warnings
- Remove in next major version
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
examples/sites/demos/apis/card.js(3 hunks)examples/sites/demos/pc/app/card/custom-class-composition-api.vue(2 hunks)examples/sites/demos/pc/app/card/custom-class.vue(2 hunks)examples/sites/demos/pc/app/card/webdoc/card.js(1 hunks)packages/theme/src/card/index.less(1 hunks)
🔇 Additional comments (7)
examples/sites/demos/pc/app/card/custom-class-composition-api.vue (2)
2-3: LGTM! Template changes improve clarity.
The simplified wrapper and semantic custom class name enhance the demo's readability while maintaining functionality.
12-13: Consider documenting the removal of interactive options.
The script has been simplified to focus on class customization, which aligns with the component's simplification goals. However, since interactive options (delete, edit, sort, share) were removed, consider updating the documentation to reflect these API changes.
examples/sites/demos/pc/app/card/custom-class.vue (2)
2-3: LGTM! Improved class naming convention.
The simplified template structure with the standardized class naming convention (demo-card-custom-class) improves code clarity and maintainability.
Line range hint 2-25: Verify alignment with updated Card API.
Since this is part of a larger card component simplification effort, let's verify that this demo accurately reflects the current API.
✅ Verification successful
The custom-class prop is properly supported in the Card component
The code review shows that the custom-class prop is properly defined and implemented in both PC and mobile-first versions of the Card component:
- It's defined in the component's props (
index.ts):customClass: String - It's used in the template for both PC and mobile-first versions
- The demo correctly demonstrates its usage for styling customization
The implementation aligns with the current Card API and no issues are found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this demo aligns with the current Card API documentation
# Look for the main Card API definition file
fd -t f "card.js" -X grep -l "export const api"
# Check for any other demos that might need similar updates
fd -t f -e vue "card" examples/sites/demos/pc/app/card
Length of output: 894
Script:
#!/bin/bash
# Let's check the Card component's source code and API documentation
# Search for Card component definition and props
ast-grep --pattern 'defineComponent({
$$$
props: {
$$$
}
$$$
})'
# Search for custom-class prop specifically
rg -A 3 "custom-class|customClass"
# Check webdoc or API documentation
fd -t f -e md "card" -X grep -l "custom-class"
Length of output: 108866
Script:
#!/bin/bash
# Let's check the Card component's source code and props specifically
# Search for Card component definition and custom-class prop
ast-grep --pattern 'customClass: {
$$$
}'
# Look for Card API documentation
rg -A 5 "custom-class|customClass" "packages/vue/src/card/src"
# Check if there are any recent changes to the Card component
rg -A 5 "custom-class|customClass" "packages/vue/src/card"
Length of output: 3713
packages/theme/src/card/index.less (1)
27-27: Verify the impact of fixed border-radius.
While simplifying to a fixed border-radius aligns with the PR's goal, this change could affect existing implementations that rely on size-specific border radiuses.
Consider:
- Documenting this as a breaking change in the PR description
- Using a CSS variable for the border-radius value to maintain flexibility:
- border-radius: 8px;
+ border-radius: var(--tv-Card-border-radius, 8px);examples/sites/demos/pc/app/card/webdoc/card.js (1)
56-176:
Verify breaking changes and update documentation accordingly.
The commented out demos indicate significant feature removals from the Card component:
- Size variations (mini, small, medium, large)
- Disabled state
- Card grouping
- Status indicators
- Selection modes (checkbox/radio)
- Operation bar
- Event handling (change, icon-click events)
These removals represent breaking changes that could impact existing implementations.
Please ensure that:
- These removals are intentional and aligned with the component's roadmap
- Breaking changes are documented in the changelog
- Migration guide is provided for users to update their implementations
examples/sites/demos/apis/card.js (1)
80-190: Clarify the permanence of API removals
The comment "以下IPI暂不暴露" (these APIs are temporarily not exposed) suggests these removals might be temporary, which conflicts with the PR's simplification objective. Additionally, some of the removed properties seem fundamental to component functionality:
disabled: Common accessibility requirementsize: Basic styling requirementstatus: Important for visual feedback
Please clarify:
- Are these removals permanent or temporary?
- How should consumers handle these common requirements without these props?
Let's verify the impact of these removals:
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 |
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
Release Notes
New Features
Cardcomponent's API to streamline properties and events for improved usability.demo-card-custom-classto enhance visual appeal.Bug Fixes
Style
border-radiusfor a more modern look.Refactor