-
-
Notifications
You must be signed in to change notification settings - Fork 516
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
refact: merge Fabric & Paper impls: ScreenStackHeaderSubview (4) #1418
Merged
kkafar
merged 177 commits into
main
from
@kkafar/merge-fabric-to-paper-screen-stack-header-subview
Apr 28, 2022
Merged
refact: merge Fabric & Paper impls: ScreenStackHeaderSubview (4) #1418
kkafar
merged 177 commits into
main
from
@kkafar/merge-fabric-to-paper-screen-stack-header-subview
Apr 28, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RNSScreenController to RNSScreen
It caused RN_FABRIC_ENABLED flag to be invalid (as it merged with previous flag from the list...)
Otherwise C++ standard headers are not visible from .m files -> project fails to build. e.g. Whem=n A.m includes B.h & B.h imports memory header: "#include <memory>" -- memory header is not visible & compilation fails
paper specific section
1. move the prop to implemented section in JS 2. move the prop to "common" section in RNSScreenView interface
specific section
specific section Aim: to ake project compile
most likely some kind of merging artifact
most likely some kind of merge artifact
temporary solution until customAnimationOnSwipe is not added to RNSScreenView on Fabric
these directives should be removed once implementations ale fully merged
to Paper specific section This method is not present in RNSScreenStackComponentView, therefore I conclude it is not necessary at least for now
Fabric specific section
This method is not exposed in class interface
directives RCTComponentViewProtocol is not available on paper
There are references to subview.bridge in RNSScreenStackHeaderConfig methods c03b809
merge artifacts!
WoLewicki
reviewed
Apr 28, 2022
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 good! I added some comments.
also remove redeclaration in implementation
This is changed when ScreenStackHeaderSubview is merged
Base automatically changed from
@kkafar/merge-fabric-to-paper-screen-stack-header-config
to
main
April 28, 2022 10:28
…g' into @kkafar/merge-fabric-to-paper-screen-stack-header-subview
This was already done in previous commit, but it somehow came back after merging base branches.
kkafar
deleted the
@kkafar/merge-fabric-to-paper-screen-stack-header-subview
branch
April 28, 2022 13:09
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
PRs mentioned just above aim to merge new Fabric components implementation to Paper.
They should be merged in ascending order (each PR has a order number associated).
Test code and steps to reproduce
Checklist