feat!: add StrictMode support #5687
Conversation
| }; | ||
|
|
||
| // Normalizes Transition callbacks when nodeRef is used. | ||
| const TransitionWrapper = React.forwardRef< |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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} |
| @@ -186,6 +181,10 @@ function OverlayTrigger({ | |||
| ...props | |||
| }: OverlayTriggerProps) { | |||
| const triggerNodeRef = useRef(null); | |||
| const mergedRef = useMergedRefs<unknown>( | |||
| triggerNodeRef, | |||
| (children as any).ref, | |||
|
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