-
Notifications
You must be signed in to change notification settings - Fork 2
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
Extract transition component handling to useTransitionComponent hook #1131
Conversation
); | ||
}, | ||
[transitionComponent, transitionComponentVisible, onTransitionEnd] | ||
); |
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.
My initial idea to hide the logic of conditionally wrapping the consuming component in a TransitionComponent
or a Fragment
was to expose an actual component:
const Wrapper = !transitionComponent ? Fragment : ({ children }) => {
const TransitionComp = transitionComponent;
return <TransitionComp ... />;
};
And then use it like this in the consuming component:
const { Wrapper, ... } = useTransitionComponent(...);
return (
<Wrapper>
<div>...</div>
</Wrapper>
);
However, that introduces a component which is defined within a component, and the effects didn't work as expected.
That's why I ended up creating a callback that does the wrapping.
361463d
to
202b853
Compare
9f5ca94
to
55fc983
Compare
Codecov Report
@@ Coverage Diff @@
## closable-dialog-transition #1131 +/- ##
============================================================
Coverage 100.00% 100.00%
============================================================
Files 57 58 +1
Lines 822 830 +8
Branches 320 320
============================================================
+ Hits 822 830 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
202b853
to
019ea6d
Compare
55fc983
to
0dd6596
Compare
019ea6d
to
13fc954
Compare
0dd6596
to
33f63d8
Compare
13fc954
to
c29884d
Compare
33f63d8
to
41d7443
Compare
Closing as per #1117 (comment) |
This PR extracts all the logic to manage
TransitionComponents
fromDialog
to a reusable hook, opening the door to use it in other places with components that can expose a similar props API, while they internally use this hook.As a good side effect, we can extract all the complexity to internally sync state to properly manage transitions, from the
Dialog
component, making it simpler.