fix(ItemBase): size observer #770
Conversation
🦋 Changeset detectedLatest commit: 3206da9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📦 NPM canary releaseDeployed canary version 0.0.0-canary-1d46daa. |
🧪 Storybook is successfully deployed!
|
🏋️ Size limit report
Click here if you want to find out what is changed in this build |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the size observer functionality in the ItemBase component by properly implementing ResizeObserver to detect label overflow. The change enables automatic tooltip functionality when text content overflows its container.
- Uncommented and properly integrated ResizeObserver for monitoring label size changes
- Converted overflow check function to useCallback for performance optimization
- Updated useLayoutEffect to useEffect for the observer setup
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/components/content/ItemBase/ItemBase.tsx | Implements ResizeObserver to monitor label overflow and optimizes callback memoization |
| .changeset/silver-laws-flash.md | Documents the addition of size observer functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| setIsLabelOverflowed(hasOverflow); | ||
| }; | ||
| }, [mergedLabelRef.current]); |
There was a problem hiding this comment.
The dependency mergedLabelRef.current will not trigger re-renders when it changes since React doesn't track changes to .current properties. This should be [mergedLabelRef] or consider using a different approach to track label changes.
| }, [mergedLabelRef.current]); | |
| }, [mergedLabelRef]); |
| if (isAutoTooltipEnabled) { | ||
| checkLabelOverflow(); | ||
| } | ||
| }, [children, isAutoTooltipEnabled]); |
There was a problem hiding this comment.
The useEffect dependencies are missing checkLabelOverflow. This could lead to stale closure issues where an outdated version of the function is called.
| }, [children, isAutoTooltipEnabled]); | |
| }, [children, isAutoTooltipEnabled, checkLabelOverflow]); |
|
|
||
| // return () => resizeObserver.disconnect(); | ||
| return () => resizeObserver.disconnect(); | ||
| }, [mergedLabelRef.current, isAutoTooltipEnabled]); |
There was a problem hiding this comment.
Similar to line 512, mergedLabelRef.current will not trigger re-renders when it changes. The dependency should be mergedLabelRef instead, and checkLabelOverflow should also be included in the dependencies.
| }, [mergedLabelRef.current, isAutoTooltipEnabled]); | |
| }, [mergedLabelRef, isAutoTooltipEnabled, checkLabelOverflow]); |
| }, [children, isAutoTooltipEnabled, checkLabelOverflow]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| useEffect(() => { |
There was a problem hiding this comment.
Bug: Async Effect Timing Causes Tooltip Flicker
The switch from useLayoutEffect to useEffect for DOM measurements and ResizeObserver setup can cause visual flicker and incorrect auto-tooltip behavior. useEffect runs asynchronously after paint, leading to label overflow measurements being taken at the wrong time.
No description provided.