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

feat: Screen props: fullScreenSwipeEnabled (1) #1362

Merged
merged 23 commits into from
Apr 1, 2022

Conversation

@kkafar kkafar changed the base branch from main to @kkafar/fabric-rnsscreen-props-base March 18, 2022 08:35
@kkafar kkafar requested a review from WoLewicki March 18, 2022 12:29
Copy link
Member Author

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Some notes for reviewer

Comment on lines +143 to +145
private fun logNotAvailable(propName: String) {
Log.w("RN SCREENS", "$propName prop is not available on Android")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should be moved out from the class to some kind of utils. This time I followed example of ScreenStackHeaderConfigViewManager

Comment on lines +8 to +11
typedef NS_ENUM(NSInteger, RNSScreenSwipeDirection) {
RNSScreenSwipeDirectionHorizontal,
RNSScreenSwipeDirectionVertical,
};
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved such enum types to the external header file in one of following PRs

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 job 🎉 I added some comments, please answer them.


- (void)didMoveToWindow
{
if (self.window != nil && ![self isMountedUnderScreenOrReactRoot]) {
Copy link
Member

Choose a reason for hiding this comment

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

It was added for proper gesture handling in modals and it would be good to check if it works correctly there after we merge modal stackPresentations. It will be good to test on example provided in #784


if (parent != nil) {
RCTSurfaceTouchHandler *touchHandler = [parent performSelector:@selector(touchHandler)];
[touchHandler setEnabled:NO];
Copy link
Member

Choose a reason for hiding this comment

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

Is calling those 2 methods the same as this:

[touchHandler cancel];
earlier? Did you test this?

Copy link
Member Author

@kkafar kkafar Mar 31, 2022

Choose a reason for hiding this comment

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

It should be. The thing is, that in new counterpart of RCTTouchHandler -- RCTSurfaceTouchHandler the cancel method became private, but it has the same impl as before (old impl, new impl).

PS. Tested -- seems to work properly

Should I put a test screen for this in FabricTestExample or maybe the test exists somewhere already? 🤔

ios/RNSScreenStackComponentView.mm Show resolved Hide resolved
@kkafar kkafar marked this pull request as ready for review March 22, 2022 13:03
@kkafar kkafar force-pushed the @kkafar/screen-props-fullScreenSwipeEnabled branch from ae55c29 to 92c1eef Compare March 25, 2022 14:36
@kkafar kkafar requested a review from WoLewicki March 31, 2022 07:38
@kkafar kkafar changed the base branch from @kkafar/fabric-rnsscreen-props-base to main March 31, 2022 07:38
@kkafar kkafar force-pushed the @kkafar/screen-props-fullScreenSwipeEnabled branch from 92c1eef to a8019a8 Compare March 31, 2022 13:42
@kkafar kkafar merged commit c1c1dd9 into main Apr 1, 2022
@kkafar kkafar deleted the @kkafar/screen-props-fullScreenSwipeEnabled branch April 1, 2022 09:37
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