-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(AbstractNav): allow passed in refs to be properly forwarded #4031
Conversation
does not currently work, due to various bugs
Co-Authored-By: Jimmy Jia <tesrin@gmail.com>
This comment has been minimized.
This comment has been minimized.
This reduces the complexity of the AbstractNav implementation, especially in regards to how the contexts are integrated with it.
This fixes the assertion issues caused by the selectors testing implementation details to find the DOM elements, rather than testing for something that is gauranteed to be in the implementation (E.g. the class `nav` on the Nav component, since it's part of the Bootstrap design spec) of the rendered DOM element.
Currently, we use a ref to gain access to the underlying component to implement our own functionality. However, this causes an issue with not allowing the user to forward their own ref to gain access to the underlying component. using `useMergedRefs`, we are able to forward both refs to the underlying component.
(Just a side-note, we should consider releasing |
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.
@jquense any reason why useMergedRefs
isn't in @restart/hooks
?
no reason, we should probably put it there! |
i updated this to use restart/hooks; merging so i can cut a release |
Thinking we’ll need to update |
sure, but we can do that later |
Abstract Nav
should be forwarding its ref to the underlying component, so thatNav
(and any other components using this) can get the proper ref.Fixes #4012.