-
Notifications
You must be signed in to change notification settings - Fork 310
fix(crop): [crop] change theme #3015
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes update the crop component's CSS styling. Fixed color values in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/theme/src/crop/index.less (1)
325-334
: Usage of CSS Variables for Dialog Handle Styling
The updated block for&__dialog-content__handle
now leverages CSS variables for both the background and box-shadow. This aligns well with the theme customization objectives. Consider adding fallback values (using the CSS syntaxvar(--variable, fallback)
) if there is any risk that these variables might not be defined.packages/theme/src/crop/vars.less (1)
20-29
: Introduction of New CSS Custom Properties
The new custom properties for the crop component (background, font, hover, box-shadow, and split background) are well defined and logically assigned. This change greatly enhances the maintainability and flexibility of the theme. One minor suggestion: the comment on line 28 appears to repeat "条框的阴影" similarly to line 26, whereas it might be clearer if the comment for line 29 specifies that it defines the split background color (e.g., "条框分割背景颜色") to avoid ambiguity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/crop/index.less
(3 hunks)packages/theme/src/crop/vars.less
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (3)
packages/theme/src/crop/index.less (3)
342-347
: Icon Button Fill Color via CSS Variable
The change to usevar(--tv-Crop-select-font-color)
for the fill property in the.iconButtonset
block is consistent with the new theming approach and improves maintainability.
352-354
: Hover Background Update with CSS Variable
Updating the hover effect background color to usevar(--tv-Crop-select-icon-hover-color)
ensures a more dynamic and theme-consistent interaction state for the icons.
357-362
: Icon Button Split Background Color Adjustment
The background color for.iconButton__split
is now driven byvar(--tv-Crop-select-box-split-bg-color)
, which is in line with the overall shift to CSS variables. The implementation is clear and consistent.
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