chore: overflow utility initial commit #18644
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18644 +/- ##
=======================================
Coverage 84.28% 84.28%
=======================================
Files 409 409
Lines 14702 14702
Branches 4846 4842 -4
=======================================
Hits 12392 12392
- Misses 2146 2147 +1
+ Partials 164 163 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is awesome! 🔥
Some pieces of the react usage example have me wondering if the abstractions you have here could be modified to improve performance and usage.
An entirely new overflowHandler is created anytime the actions change. actions may be unlikely to change, but it seems like we should be able to reuse the one we already have. The container isn't changing, none of the other "props" are changing, the existing resize observer doesn't need to be destroyed, etc.
Also the amount of boilerplate stood out to me. It would be best if the consumer didn't have to bear that burden.
If the internal functions (getDimension, update, etc) were available by themselves in addition to createOverflowHandlder, we could make a useOverflowHandler hook for react users that uses these internally. It would streamlines the usage for consumers while also giving us a chance to make it more performant. We could memoize the actions and the container, contain the hiddenItems state, selectively provide return values if they were to need them, etc.
It might even be that we could avoid handling the requestAnimationFrame ourselves and use useLayoutEffect in the hook.
You've maybe considered all this though 😅 if so, we can divert to a new issue. Either way I'm curious to hear what you think
| /** | ||
| * Maximum number of visible items. If provided, only this number of items will be shown. | ||
| */ | ||
| maxVisible?: number; |
There was a problem hiding this comment.
| maxVisible?: number; | |
| maxVisibleItems?: number; |
|
hey @tay1orjones , i like to do some more changes to this, to reduce the boilerplate, i see some potential for it, and yes, actionbar example doesnt need any memoization, i just copy pasted the example from tagset and forgot to clean up, since these are just vague examples and we are yet to build proper usage examples on lit and react. which is going to be a seperate issue. we are destroying and recreating the overflowHandler on tagset overflow tag width changes which usually happens at number 9->10, 99->100. ill move getDimension (will change this to getSize) outside the handler and export it for reusability, i should also add a note to it, not to call it in any loop or update function, as they are expensive and could clog the memory usage, it could be better to call it once per instance, and revoke and recall the instance if we want to recalculate it. ill move this to draft and will open with new changes soon. thanks for the review, it enabled few more insights to this. |
|
Hi @tay1orjones , this is done, and open for re-review iteration 2 change log
|
Closes #6883 on ibm-products
adds framework agonistic overflow utility, to the utility package.
Changelog
New
Testing / Reviewing
automated tests written in the test file
existing ibm-products actionbar and tagset example
https://stackblitz.com/edit/github-yxe1kwrz?file=src%2FExample%2FExample.jsx
Iteration 1
React usage example of this utility
Lit usage example of this utility
iteration 2
React usage example of this utility
Lit usage example of this utility