-
-
Notifications
You must be signed in to change notification settings - Fork 600
feat: Remove native-stack v5 from screens repo #3433
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
Conversation
8e653e6 to
1c9f539
Compare
kkafar
left a comment
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.
Guide: this is a feature related change, as it modifies public interface of the library, therefore use feat as the commit type in the PR title.
kkafar
left a comment
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.
Looks nice. I just have few comments.
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.
Does this test case even work? I think that we never downstreamed this feature to react-navigation. There was a PR from @maciekstosio, but it has not been completed.
Please verify whether we have ticket for downstreaming it (I think I recently saw one on the board, it could have been a draft so it won't be visible in labs issues). If not -> create one.
Now, regarding this test -> if it does not work, it should be "suspended". I'd recommend commenting it out in index file, so that it is not loaded or marking it somehow as "disabled" (can be done in separate, follow up PR).
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.
This is the PR:
react-navigation/react-navigation#12225
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.
kkafar
left a comment
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.
Okay, let's do it. ✅
## Description This PR removes TestScreenAnimationV5. This test is showing a functionality that is not completed on react-navigation side. ## Changes Commented out TestScreenAnimationV5. ## Test code and steps to reproduce Check if TestScreenAnimationV5 is present in the Example list, it should not.
Description
This PR removes native-stack directory from screens repo. This dir held the code for native-stack v5, which has been moved to react-navigation and is v7, v8 now. The rest of the deprecated imports are removed and the unused code is deleted.
Closes https://github.com/software-mansion/react-native-screens-labs/issues/589
Changes
Removed native-stack/*, fixed imports.
Test code and steps to reproduce
Check if the library still builds successfully.