Skip to content
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: fix warnings in strict mode #97

Merged
merged 1 commit into from
Mar 15, 2024
Merged

fix: fix warnings in strict mode #97

merged 1 commit into from
Mar 15, 2024

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

@kyletsang kyletsang merged commit e900a9b into main Mar 15, 2024
5 of 8 checks passed
@kyletsang kyletsang deleted the fix-strict-mode branch March 15, 2024 19:04
@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
3 participants