Skip to content

Conversation

@kyletsang
Copy link
Collaborator

Alternative implementation to #95 with no breaking changes. Adapts same approach we made in RB in normalizing transition callbacks when using nodeRef.

Fixes #93

@kyletsang kyletsang requested a review from jquense March 14, 2024 05:10
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeeeesss

@pzaczkiewicz-athenahealth
Copy link

pzaczkiewicz-athenahealth commented Mar 15, 2024

Thank you! When I coded #95 I couldn't see a way to implement it without breaking Typescript changes. I hadn't considered adding a new hook. So to use the new system, I'd change:

const FadeUp = (props: FadeProps): ReactElement => {
  const { children, ...transitionProps } = props;
  return (
    <CSSTransition timeout={400} classNames="fade-up" {...transitionProps}>
      {children}
    </CSSTransition>
  );
};

to

const FadeUp = (props: FadeProps): ReactElement => {
  const { children, ...transitionProps } = useRTGTransitionProps(props);
  return (
    <CSSTransition timeout={400} classNames="fade-up" {...transitionProps}>
      {children}
    </CSSTransition>
  );
};

Is that correct?

@kyletsang
Copy link
Collaborator Author

Actually the latest update will work with no changes on your side. Internally we use the hook for you and apply the normalized transition props.

I'll cut a release soon, just taking care of some CI stuff

@kyletsang
Copy link
Collaborator Author

Released in 1.6.7

@pzaczkiewicz-athenahealth

Can confirm that I no longer get this console warning. I also agree that your diff looks less scary than mine, but I think they're effectively the same. My code sets nodeRef at the time children was created, rather than "steal" that prop from the ReactComponent. Either way it works!

The breaking change that I thought you were referring to was the bifurcation of TransitionProps into TransitionProps and ReactTransitionGroupProps. I had done that because I wanted the type for OverlayProps['transition'], ModalProps['transition'] and ModalProps['BackdropTransition'] to be as accurate as possible, including the inclusion of nodeRef. This PR doesn't change the type definitions, so nodeRef is effectively an undocumented prop. However, everything works in runtime so long as leftover props are spread across the CSSTransition.

I'm spreading these props anyway, and I care more about the runtime behavior than 100% Typescript accuracy, but just wanted to flag this issue.

Once again, thanks for implementing and releasing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modal and Overlay generate findDOMnode warnings when using transitions

4 participants