-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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: add additional check before running animate
in componentDidUpdate
#10871
fix: add additional check before running animate
in componentDidUpdate
#10871
Conversation
This is required to support the new Fabric renderer
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
The Expo app for the example from this branch is ready! expo.dev/@react-navigation/react-navigation-example?release-channel=pr-10871 |
Codecov ReportBase: 75.47% // Head: 75.43% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #10871 +/- ##
==========================================
- Coverage 75.47% 75.43% -0.04%
==========================================
Files 167 167
Lines 5129 5130 +1
Branches 1985 1986 +1
==========================================
- Hits 3871 3870 -1
- Misses 1220 1222 +2
Partials 38 38
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
### Motivation This PR is a third attempt to migrate setNativeProps to state in @react-navigation/stack following the official Moving setNativeProps to state guide. After issues with the implementation of #10767 and more problems with the fix in #10871 we had to take a step back and rethink where the problem lays with these implementations. The solution was to move the state inside the CardSheet component and use useImperativeHandle to hoist a setter function up to Card to avoid triggering a rerender during Card animate function.
### Motivation This PR is a third attempt to migrate setNativeProps to state in @react-navigation/stack following the official Moving setNativeProps to state guide. After issues with the implementation of #10767 and more problems with the fix in #10871 we had to take a step back and rethink where the problem lays with these implementations. The solution was to move the state inside the CardSheet component and use useImperativeHandle to hoist a setter function up to Card to avoid triggering a rerender during Card animate function.
…entDidUpdate` (react-navigation#10871)" This reverts commit b9fb2d1.
### Motivation This PR is a third attempt to migrate setNativeProps to state in @react-navigation/stack following the official Moving setNativeProps to state guide. After issues with the implementation of react-navigation#10767 and more problems with the fix in react-navigation#10871 we had to take a step back and rethink where the problem lays with these implementations. The solution was to move the state inside the CardSheet component and use useImperativeHandle to hoist a setter function up to Card to avoid triggering a rerender during Card animate function.
Motivation
Changed introduced in #10767 created a regression reported in #10856 which made it impossible to close a modal with a gesture.
As we want to change pointer-events on Card in
componentDidUpdate
previous use ofsetNativeProps
in this context made a lot of sense. SincesetNativeProps
is going to be removed in the new architecture we had to migrate it to the component state as stated in the migration guide.This PR adds an additional
closing !== prevProps.closing
check on top of #10767 withincomponentDidUpdate
inCard.tsx
.Test plan
ModalStack example
Recordings
Before
Screen.Recording.2022-09-21.at.12.35.45.mov
After
Screen.Recording.2022-09-21.at.12.34.51.mov
Issue the aforementioned PR fixed originally still works
Screen.Recording.2022-09-21.at.12.33.11.mov