-
Notifications
You must be signed in to change notification settings - Fork 144
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
refactor(ActionBar): implement useOverflowItems hook in ActionBar #6775
Conversation
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ibm-products-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Screen.Recording.2025-01-29.at.8.43.37.AM.mov
looks like there's still something not quite right here. i'll take a look and help you to identify what's causing this stutter
also tests are failling
I've tested this approach, but it is not working as expected in most of the cases. Screen.Recording.2025-02-07.at.12.36.48.PM.mov |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6775 +/- ##
==========================================
+ Coverage 81.49% 81.50% +0.01%
==========================================
Files 399 399
Lines 12978 12991 +13
Branches 4270 4282 +12
==========================================
+ Hits 10576 10588 +12
- Misses 2402 2403 +1
|
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.
i think a bit more than what was needed was added in the last commit. i don't see the added benefit to the additional effects or refs. i think the only thing that was needed to fix the issue with the offset not being initially calculated when not present. just the inclusion of that example code i sent on friday
const offsetRefHolder = useRef<number | null>(null);
const handleResize = () => {
if (containerRef.current) {
const offset = offsetRef?.current?.offsetWidth || 0;
if (offset && !offsetRefHolder.current) {
offsetRefHolder.current = offset;
}
const newMax =
containerRef.current.offsetWidth - (offsetRefHolder.current || 0);
setMaxWidth(newMax);
}
};
if i'm missing something with the new implementation please let me know.
} | ||
}, [getVisibleItems, onChange, remainingWidth, requiredWidth]); | ||
|
||
useEffect(() => { |
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.
i don't think that this effect is necessary. i don't think we need to include these new refs for hidden and visible items. if we can calculate these things on the fly then there's no real need to store them anywhere unless we're trying to cache the value.
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.
You're right, I've given a second look and refactored that.
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.
ok looks good! i think we're ready to move on
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.
LGTM
f7609f5
Closes #6720
What did you change?
useOverflowItems
hook inActionBar
OverflowMenu
issueActiobar
width issuePageHeader
How did you test and verify your work?
Storybook
yarn test