-
Notifications
You must be signed in to change notification settings - Fork 4
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
[WEB-4100] TabMenu animation delay and root/contents class modifiers #581
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces two main changes: an update to the package version in Changes
Assessment against linked issues
Poem
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 (
|
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)
src/core/TabMenu.tsx (2)
83-83
: Consider extracting the animation delay constant.The animation logic is sound, but the hardcoded delay of 150ms should be extracted into a named constant for better maintainability and documentation.
+ const ANIMATION_DELAY_MS = 150; useEffect(() => { if (animatedOption && highlight.width > 0) { setTimeout(() => { setAnimated(true); - }, 150); + }, ANIMATION_DELAY_MS); } }, [animatedOption, highlight.width]);Also applies to: 89-89, 92-98
142-142
: Consider adding type safety for className props.While the className application is correct, consider using a more type-safe approach for className props to prevent runtime errors.
import { ClassValue } from 'clsx'; export type TabMenuProps = { // ... other props rootClassName?: ClassValue; contentsClassName?: ClassValue; };This would ensure that only valid class values can be passed to the component.
Also applies to: 183-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json
(1 hunks)src/core/TabMenu.tsx
(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (3)
src/core/TabMenu.tsx (3)
30-39
: LGTM! Well-documented prop additions.
The new rootClassName
and contentsClassName
props are well-documented and follow the existing TypeScript patterns.
76-77
: LGTM! Clean props destructuring.
The new props are correctly destructured alongside existing props.
Line range hint 1-194
: Verify animation behavior across different scenarios.
Please ensure the animation behavior is tested in the following scenarios:
- Initial render
- Tab switching
- Window resize
- Dark/light theme switches
- Different viewport sizes
f20004c
to
c1d7f60
Compare
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.
c1d7f60
to
3b7e19d
Compare
3b7e19d
to
f2af5c4
Compare
b456c7a
to
d96b56b
Compare
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.
just had a look as well and its looks nice!
Jira Ticket Link / Motivation
https://ably.atlassian.net/browse/WEB-4100
Summary of changes
Fixes an issue with
TabMenu
where the animated highlight under the active tab extends outwards from 0px to the full width of the tab, meaning that every re-render, you get the horizontal panning in the video below. Not ideal, so this PR only "turns on" the animated width CSS transition after the target width of the highlight has been calculated, meaning no extra transition at the start, but transitions are maintained when navigating from tab to tab.Screen.Recording.2024-12-17.at.16.31.50.mov
Also adds extra props to modify the class names of the root and contents elements in
TabMenu
, and adds an extra icon needed forLeftSidebar
.Version bump:
package.json
from15.1.8
to15.1.9
.How do you manually test this?
Book
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
TabMenu
component with customizable class names for root and content elements.TabMenu
component.Version Update
15.1.9
.