fix(dropdown): [dropdown] fix icon shaking when hover #2557
Conversation
WalkthroughThe changes made in this pull request involve modifications to the CSS styles of the dropdown component, specifically 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 (2)
packages/theme/src/dropdown/vars.less (1)
18-18: Consider documenting hover state best practices.Since this fix addresses icon shaking on hover, consider:
- Adding a comment explaining the importance of consistent margins for hover stability
- Creating a shared documentation or guideline for handling hover states across components to prevent similar issues
packages/theme/src/dropdown/index.less (1)
Line range hint
143-144: Consider translating the Chinese comment to English for better maintainability.The comment explains an important implementation detail about using
scaleY(-1)instead ofrotate(180deg)to prevent position shifts.- /* 下拉图标旋转 */ + /* Dropdown icon rotation */ &--visible { - transform: scaleY(-1); // rotate(180deg) 会受svg的margin影响，位置会变动！ + transform: scaleY(-1); // Using scaleY instead of rotate(180deg) to prevent position shifts caused by SVG margins transform-origin: center; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/theme/src/dropdown/index.less(2 hunks)packages/theme/src/dropdown/vars.less(1 hunks)
🔇 Additional comments (3)
packages/theme/src/dropdown/vars.less (1)
18-18: Good improvement in variable naming, but let's verify the complete fix.
The renaming from --tv-Dropdown-icon-margin-left to --tv-Dropdown-icon-margin-x is a good change as it:
- Provides more flexibility for bidirectional layouts (LTR/RTL)
- Allows for consistent horizontal spacing
Let's verify if this change, combined with its usage in index.less, properly addresses the icon shaking:
✅ Verification successful
Icon shaking fix is properly implemented with consistent margins
The verification shows the new --tv-Dropdown-icon-margin-x variable is correctly applied:
- Used for
margin-rightin prefix elements - Used for
margin-leftin suffix elements - No traces of the old variable name remain
- The suffix inner element has proper flex alignment
The implementation ensures stable icon positioning by:
- Using consistent margins on both sides
- Proper flex alignment with
display: inline-flexandalign-items: center
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the new margin variable is applied and if there are any related layout styles that might affect icon stability
# Search for the old variable name to ensure complete migration
rg --type=less "tv-Dropdown-icon-margin-left"
# Check usage of the new variable and surrounding layout styles
rg --type=less -A 5 "tv-Dropdown-icon-margin-x"
# Look for other potential layout-affecting styles
ast-grep --pattern 'tiny-dropdown__suffix-inner {
$$$
}'
Length of output: 1418
packages/theme/src/dropdown/index.less (2)
30-42: LGTM! These changes effectively fix the icon shaking issue.
The modifications address the icon shaking by:
- Using
inline-flexwithalign-items: centerensures stable vertical alignment - Consistent margin handling through the new inner wrapper classes prevents layout shifts
79-79: Verify button group layout changes across different scenarios.
The change from display: block to display: inline-flex aligns well with the flex-based approach. However, please verify this change doesn't affect existing button group layouts.
✅ Verification successful
Change from display: block to display: inline-flex is safe to proceed
The verification shows this change is safe because:
- The main
button-groupcomponent already usesdisplay: inline-blockin its base styles (packages/theme/src/button-group/index.less) - Only the SAAS theme has an explicit
display: blockoverride (packages/theme-saas/src/dropdown/index.less), but this is a separate theme package - No visual regression tests indicate layout-specific issues with button groups
- The change aligns with the component's existing inline display behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom overrides or dependencies on block display
rg -l "\.tiny-button-group\s*{[^}]*display:\s*block" --type less
# Search for potential layout issues in test files
rg -l "button-group.*layout" --type spec
Length of output: 408
Script:
#!/bin/bash
# Search for any custom overrides or dependencies on block display
rg -l "\.tiny-button-group.*display:\s*block"
# Search for button group related test files
rg -l "ButtonGroup.*test"
# Check for any existing button group styles
rg "button-group" --type less -A 3 -B 3
# Look for any visual regression tests
fd -e spec.ts -e spec.tsx -e test.ts -e test.tsx | xargs rg "ButtonGroup"
Length of output: 8946
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