fix(Button): fixing animated label logic #1422
Conversation
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
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: 1
🧹 Outside diff range and nitpick comments (1)
packages/react-components/src/components/Button/styles/animated-label.scss (1)
53-72: Consider DRYing up the hover/focus-visible selectorsThe padding adjustments could be consolidated using a shared selector, but the current explicit approach is also valid for maintainability.
&.#{$base-class}--with-left-icon, &.#{$base-class}--with-right-icon { padding-right: $padding-default; padding-left: $padding-default; + &:is(:hover, :focus-visible) { + padding-right: $padding-hover-label; + padding-left: $padding-hover-icon; + } - &:hover, - &:focus-visible { - padding-right: $padding-hover-label; - padding-left: $padding-hover-icon; - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/react-components/src/components/Button/Button.tsx(2 hunks)packages/react-components/src/components/Button/styles/animated-label.scss(3 hunks)
🔇 Additional comments (4)
packages/react-components/src/components/Button/styles/animated-label.scss (2)
7-7: Great implementation of CSS-based animation!
The use of CSS variable for width control and inclusion of focus-visible state improves both performance and accessibility.
Also applies to: 10-17
31-31: Perfect fix for the expansion bug!
Setting initial max-width to 0 prevents unwanted expansion, directly addressing issue #1364.
packages/react-components/src/components/Button/Button.tsx (2)
7-7: LGTM! Simplified imports align with CSS-based interaction approach
The removal of keyboard/mouse interaction handlers in favor of CSS pseudoclasses is a good architectural decision.
136-138: Verify button behavior during tab switching
The CSS-based approach should prevent automatic expansion, but please verify:
- Button doesn't expand automatically when switching tabs
- Label appears only on hover/focus
- Animation works smoothly in all scenarios
✅ Verification successful
CSS-based implementation confirmed safe for tab switching
The verification shows:
- No state-based width control (no useState hooks found)
- Pure CSS transitions using
var(--button-label-width)andmax-width - Proper hover/focus styles with smooth transitions (200ms cubic-bezier)
- Clean implementation using CSS variables and pseudoclasses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining expansion-related code that might interfere
rg -g '!*.{json,md}' -i '(expand|collapse|isLabelOpen)' packages/react-components/src/components/Button/
Length of output: 106
Script:
#!/bin/bash
# Let's check the Button component implementation for any state-based width control
ast-grep --pattern 'const [$_] = useState<$_>($$$)' packages/react-components/src/components/Button/Button.tsx
# Check for any dynamic width-related styles or classes
rg -g '*.{ts,tsx,css,scss}' '(width|transform|transition)' packages/react-components/src/components/Button/
# Look for any hover/focus related styles
rg -g '*.{ts,tsx,css,scss}' '(:hover|:focus)' packages/react-components/src/components/Button/
Length of output: 6367
| ? // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-expect-error | ||
| { '--button-label-width': `${labelWidth}px` } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding proper TypeScript types instead of suppressing errors
While the CSS variable implementation is good, using @ts-expect-error is not ideal. Consider adding proper type definitions:
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment
- // @ts-expect-error
- { '--button-label-width': `${labelWidth}px` }
+ { ['--button-label-width' as string]: `${labelWidth}px` }Or create a proper type:
type CSSProperties = React.CSSProperties & {
'--button-label-width'?: string;
};
|
@marcinsawicki I'm closing this PR because it is not working as expected. You can reopen or duplicate it later. |
Resolves: #1364
Description
Using SCSS variables to pass calculated content width from JS to SCSS, so it will be animated relying only on CSS pseudoclasses.
Storybook
https://feature-1364--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
New Features
Bug Fixes
Style