-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: update ref handling in ListBoxMenuItem useIsTruncated hook #18611
base: main
Are you sure you want to change the base?
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18611 +/- ##
==========================================
+ Coverage 83.98% 84.96% +0.98%
==========================================
Files 408 408
Lines 14501 14556 +55
Branches 4729 4769 +40
==========================================
+ Hits 12178 12368 +190
+ Misses 2155 2026 -129
+ Partials 168 162 -6 ☔ View full report in Codecov by Sentry. |
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.
Should useIsTruncated
house the ref inspection? I'm anticipating it's soon going to be moved out of this file to be something we can import into various other components that need it.
|
||
if (element) { | ||
setIsTruncated(element.offsetWidth < element.scrollWidth); | ||
} | ||
}, [ref, ...deps]); |
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.
At first glance I thought this was an issue because changing the value of the ref won't trigger a re-render, but turns out that's not the case. TIL!
// Try to get a RefObject from `forwardedRef.menuItemOptionRef`. | ||
// If it's a callback ref (or not provided), fall back to `localRef`. |
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.
Would it make sense to test this with a mocked callback ref function?
carbon/packages/icon-build-helpers/src/builders/react/components/__tests__/Icon-test.js
Lines 40 to 47 in 7b9a05b
it('should forward refs to the rendered SVG DOM element', () => { | |
let svg; | |
const ref = jest.fn((node) => { | |
svg = node; | |
}); | |
const { container } = render(<Icon ref={ref} />); | |
expect(svg).toEqual(container.querySelector('svg')); | |
}); |
Let me know if what's in dd403b6 is better or worse. I tried to make it more |
Closes #18459
Updated
ref
handling inListBoxMenuItem
useIsTruncated
hook.Changelog
Changed
useIsTruncated
to a generic hook with explicitHTMLElement
typing.useIsTruncated
to accurately detect truncation.Testing / Reviewing
I'm not sure how to write a test for this functionality. I did take the code from #18459 and test it to ensure thattitle
s were added to the appropriate items.yarn test packages/react/src/components/ListBox