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
feat!: add StrictMode support #5687
Conversation
}; | ||
|
||
// Normalizes Transition callbacks when nodeRef is used. | ||
const TransitionWrapper = React.forwardRef< |
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 can never remember what RTG is doing now...do we need this wrapper? If it's in v5 can we just break the API and require node ref compatible components?
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.
RTG has this quirk where the node is excluded in the callback signature when using nodeRef.
ie. (node, isAppearing) => ...
becomes (isAppearing) => ...
This wrapper abstracts all that so callers don't have to worry about it (like they can just use it like it was without using nodeRef). I kinda figured this would be a cleaner approach.
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.
ah, that seems like a bad API choice i probably ok'd oops
@@ -229,19 +230,20 @@ const Collapse = React.forwardRef( | |||
onEntered={handleEntered} | |||
onExit={handleExit} | |||
onExiting={handleExiting} | |||
childRef={(children as any).ref} |
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.
this will always be falsey, children, if it's an element would have ref on children.props
not on the element itself
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.
Hmm children.ref
is actually defined when I was testing this. I've tried children.props.ref
but I get a warning:
Warning: div: ref
is not a prop. Trying to access it will result in undefined
being returned. If you need to access the same value within the child component, you should pass it as a different prop.
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.
tbh, children.ref did seem kinda hacky (esp. with the casting). I'm not 100% sure on what the correct approach is :P
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.
o hmm i must have it backwards, been doing this for too long. I'm suprised the types don't correctly cover this tho
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 found this on the react github project that seems to confirm the children.ref
usage:
ref={ref} | ||
addEndListener={transitionEndListener} | ||
{...props} | ||
onEnter={handleEnter} | ||
childRef={(children as any).ref} |
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.
ditto
@@ -186,6 +181,10 @@ function OverlayTrigger({ | |||
...props | |||
}: OverlayTriggerProps) { | |||
const triggerNodeRef = useRef(null); | |||
const mergedRef = useMergedRefs<unknown>( | |||
triggerNodeRef, | |||
(children as any).ref, |
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.
ditto
This will only be in v5, not v4, correct? |
Correct |
Looking at the tags I see 1.5.2 and 2.x, what are v5 and v4? |
V4 and v5 corresponds to the bootstrap versions, so in terms of react bootstrap this would be 1.x and 2.x respectively. |
I was under the impression that |
@augustjanse, try passing a function as a child to OverlayTrigger. See: https://react-bootstrap.netlify.app/components/overlays/#customizing-trigger-behavior |
Oh! As you might have guessed, I didn't realize that the other way of doing it would still trigger the warnings. This worked fine, thanks! |
An attempt at fixing strict mode issues for transitions, carousel and overlay trigger.
Fixes #5519
Fixes #5075