-
Notifications
You must be signed in to change notification settings - Fork 143
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
base: main
Are you sure you want to change the base?
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
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.
an additional comment regarding the offset. i would take a second look at the changes you added and make sure that they are working correctly.
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.
looking great! got a few comments 👍 🚀
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.
great! looks like a test is failing. you just need to update the tests are this should be good to go.
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 figured out the solution to dealing with the offset ref not being defined. instead of passing that ref to ActionBarOverflowItems
just wrap that component with a div that has the ref.
<div className="hidden-items" ref={offsetRef}>
{hiddenItems && hiddenItems?.length > 0 && (
...
)}
</div>
this should ensure the offset it set correctly and will make the hidden item sizing unnecessary.
Screen.Recording.2025-02-06.at.4.02.43.PM.mov
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.51% 81.50% -0.02%
==========================================
Files 399 399
Lines 12991 13003 +12
Branches 4275 4290 +15
==========================================
+ Hits 10590 10598 +8
- Misses 2401 2405 +4
|
Closes #6720
What did you change?
useOverflowItems
hook inActionBar
OverflowMenu
issueActiobar
width issuePageHeader
How did you test and verify your work?
Storybook
yarn test