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

refact: merge Fabric & Paper impls: ScreenView & ScreenController (1) #1415

Merged
merged 52 commits into from
Apr 22, 2022

Conversation

kkafar and others added 29 commits April 8, 2022 11:38
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
1. move the prop to implemented section in JS
2. move the prop to "common" section in RNSScreenView interface
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
…ansion/react-native-screens into @kkafar/merge-fabric-to-paper
@kkafar kkafar marked this pull request as ready for review April 21, 2022 10:49
@kkafar kkafar requested a review from WoLewicki April 21, 2022 10:49
@kkafar kkafar changed the title refact: merge Fabric & Paper impls: ScreenView (1) refact: merge Fabric & Paper impls: ScreenView & ScreenController (1) Apr 21, 2022
Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work 🎉 I added some comments for the future PRs.

ios/RNSConvert.h Outdated Show resolved Hide resolved
ios/RNSScreen.mm Outdated Show resolved Hide resolved
ios/RNSScreen.mm Show resolved Hide resolved
ios/RNSScreen.mm Outdated Show resolved Hide resolved
ios/RNSScreen.mm Show resolved Hide resolved
@kkafar kkafar merged commit ff61763 into main Apr 22, 2022
@kkafar kkafar deleted the @kkafar/merge-fabric-to-paper branch April 22, 2022 10:39
mccoyplayer pushed a commit to mccoyplayer/reactScreen that referenced this pull request Feb 9, 2024
… (#1415)

* chore: Set RN_FABRIC_ENABLED compiler flag when fabric is enabled

* refract: Move RNSScreenComponentView implementation to RNSScreenView &&
RNSScreenController to RNSScreen

* refract: Rename dependencies to make previous change work

* chore: update Cocoapods to .3

* fix: type in RNSScreen.mm

* fix: add missing space to compiler flags in RNScreens.podspec

It caused RN_FABRIC_ENABLED flag to be invalid (as it merged with
    previous flag from the list...)

* refact: rename all source .m files to .mm

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

* chore: update FabricExample's Podfile.lock

* fix: move reactSetFrame: method to Paper specific section

* fix: fix typo in ifdef directive

* fix: temporary: move setActivityStateOrNil & setPointerEvents methods to
paper specific section

* fix: add missing replaceAnimation prop (NOT TESTED)

1. move the prop to implemented section in JS
2. move the prop to "common" section in RNSScreenView interface

* fix: move notifyFinishTransitioning method to Paper specific section

* fix: move notifyDismissCancelledWithDismissCount: method to Paper
specific section

* fix: move notifyTransitionProgress:closing:goingForward: method to Paper
specific section

Aim: to ake project compile

* fix: remove duplication of prop setting

most likely some kind of merging artifact

* chore: change handling of unsupported props to noop

* fix: move RNSScreenManager impl & def under !RN_FABRIC_ENABLED condition

* fix: remove duplicated prop update in updateProps method

most likely some kind of merge artifact

* fix: temporary: exclude usages of customAnimationOnSwipe from Fabric

temporary solution until customAnimationOnSwipe is not added to
RNSScreenView on Fabric

* fix: make the project compile

these directives should be removed once implementations ale fully merged

* chore: restore NS_ASSUME_NONNULL_{BEGIN,END} macros

* refact: apply linter suggestions

* fix: restore RNSScreenManager on Fabric

* refact: apply linter suggestion

* fix: remove merge artifact

* fix: add *.mm sources to TestExample project

* fix: make TestsExample compile

Notice change of RNSScreenView#controller type from UIViewController to
RNSScreen

* potentially important: remove RNSScreen *_controller field

from RNSScreenView implementation

* chore: move setViewToSnapshot & resetViewToScreen methods to Fabric
specific section

* chore: merge viewWillAppear: methods

* chore: merge viewWillDisappear methods

* chore: merge viewDidAppear methods

* chore: merge viewDidDisappear methods

* chore: merge viewDidLayoutSubviews methods

* chore: move findFirstResponder method to paper specific section

* chore: move willMoveToParentViewController method to paper specific
section

* chore: move hideHeaderIfNecessary method to paper specific section

* chore: move traverseForScrollView: method to paper specific section

* chore: move methods connected to transitioning to paper specific section

temporary solution until it is not migrated

* chore: remove leftover comments

* refact: rename BASE_VIEW -> RNS_BASE_VIEW

* refact: apply linter suggestions

* refact: rename EXPECTED_VIEW -> RNS_EXPECTED_VIEW§

* chore: restore back initWithBridge method for Fabric

Fabric implementation does not need it, but RNSScreenManager is required

* fix: restore back notifyFinishTransitioning method for paper

* fix: tvOS: exclude props not existion on tv platform

* chore: remove unnecessary header guard directives from RNSConvert.h

* chore: remove check if passed view is of class RNSScreenView

see: software-mansion/react-native-screens#1415 (comment)

Co-authored-by: Wojciech Lewicki <wojciech.lewicki@swmansion.com>
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.

2 participants