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: move react-native-window-view to screens package #1096

Merged
merged 5 commits into from
Sep 8, 2021

Conversation

WoLewicki
Copy link
Member

@WoLewicki WoLewicki commented Sep 1, 2021

Description

Moved the WindowView from https://github.com/WoLewicki/react-native-window-view to this repo and renamed it to RNSFullWindowOverlay.

Changes

Added RNSFullWindowOverlay files to JS and iOS files. Removed the necessity of adding changes to AppDelegate.m by providing an extension of UIWindow in the package.

Test code and steps to reproduce

Test1096.tsx

Checklist

@WoLewicki WoLewicki linked an issue Sep 1, 2021 that may be closed by this pull request
@WoLewicki WoLewicki changed the title feat: move the view to screens package feat: move react-native-window-view to screens package Sep 1, 2021
@nandorojo
Copy link

@WoLewicki thank you for the great work on this. I think this feature could be a huge addition to React Native iOS in general.

I have a few questions:

  1. Can you use multiple WindowView components? If so, will they work like the <Modal />, where the ones rendered last will go on top?
  2. Does it matter where the WindowView is rendered? For example, rather than putting it at the root of my app, can I treat it like a Modal and put it anywhere?

A big use-case I'm thinking about here is creating custom components like Popovers, Menus, Alerts, etc. On iOS, this has always been a challenge: the only solution with a good DX is to use the native Modal, but on iOS, you can't open more than one Modal at a time. So I'm wondering if this component could finally be the solution.

Thanks again!

@WoLewicki
Copy link
Member Author

WoLewicki commented Sep 2, 2021

Ad 1. as you can see here: https://github.com/software-mansion/react-native-screens/pull/1096/files#diff-174f03b4ee9b6d6719c80e57d200b2057b704709f1b3f83218e31b54819d22d4R6 the RNSOverlayViewContainers are brought to front in the order in which they were added, so yeah, it should work like that.

Ad 2. as long as the view is not destroyed by React, it should stay in the native view hierarchy, so I think you can think of it as kind of similar thing to Modal.

Probably the best thing you can do is to test it by yourself 🎉 and see if there are any problems.

@nandorojo
Copy link

This PR might be an underrated solution to many of my iOS issues...thank you so much @WoLewicki! I will definitely test it.

A bit of a naïve question here, what is the best way to test this PR in an app?

@nandorojo
Copy link

One other question, related to the previous conversation about pointerEvents="box-none". Is this behavior possible?

<>
  <App />
  <OverlayView interceptTouches={false} shown>
    <Notifications /> 👈 this should still intercept touches when it renders
  </OverlayView>
</>

Ideally, its children can intercept touches, even if the overlay itself doesn't.

@WoLewicki
Copy link
Member Author

One way of testing is adding this package to your project in package.json straight from this commit, something like this: "react-native-screens": "github:software-mansion/react-native-screens#ede07fb". Another way is to clone the repo, checkout to this branch and apply changes inside e.g. TestsExample project to test your use-cases.

As for the behavior you mentioned, I believe it is not possible now due to simple logic enabling/disabling the whole view hierarchy of OverlayView, but it could be changed with some effort for sure and I will try to do it when I get some free time.

@WoLewicki
Copy link
Member Author

@nandorojo can you check the newest commit? It should make the whole view not intercept touches, but its children should intercept them now. I added the behavior in the example, where the OverlayView takes the whole screen and you can click on views below it if there is none of its child there. If it works and does not destroy any of react-native's behaviors, I think we can merge it. It would then probably make it to next Expo SDK.

@nandorojo
Copy link

Looks good! I'll test it out. Would be great for it to make SDK 43.

@arberkryeziu
Copy link

arberkryeziu commented Sep 6, 2021

@WoLewicki I tested this. Works very good for me. A much-needed feature

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Left two comments inline. Also, I think it'd be good to cleanup draggable/shown parameters as we discussed.

@@ -329,6 +337,14 @@ module.exports = {

return ScreensNativeModules.NativeSearchBar;
},
get OverlayView() {
Copy link
Member

Choose a reason for hiding this comment

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

I'd perhaps change the name to sth like "FullWindowOverlay" to make it cleaner that it does not just overlay the container where its put or the screen under which it is rendered but instead it displays in front of all the content in the window.

Copy link

@nandorojo nandorojo Sep 8, 2021

Choose a reason for hiding this comment

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

WindowOverlay could work too. A bit simpler to write and remember. But either is fine.

/>
</View>
</Modal>
<OverlayView style={{position: 'absolute', width: '100%', height: '100%', justifyContent: 'center' }} shown={shown} draggable={draggable} interceptTouches={interceptTouches}>
Copy link
Member

Choose a reason for hiding this comment

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

What happens if there are more than one of such components rendered in the tree?

Copy link
Member Author

Choose a reason for hiding this comment

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

They will be rendered on top of each other in the same order as they were added to React view hierarchy at initialization since this code will be called: https://github.com/software-mansion/react-native-screens/pull/1096/files#diff-4882eafb07e471e6bd8bf6558dca65eacab42001de16e1dc1ca7b23c54ab765fR108 and then the RNWindow will move all containers to front in order they were added: https://github.com/software-mansion/react-native-screens/pull/1096/files#diff-174f03b4ee9b6d6719c80e57d200b2057b704709f1b3f83218e31b54819d22d4R11

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Looks fine for the first iteration. Looking forward to getting full platform compatibility for this one!

@nandorojo
Copy link

Looking forward to getting full platform compatibility for this one!

Me too, this one is big time!

@jakob-p
Copy link

jakob-p commented Oct 18, 2021

When i try to use this with expo im getting requireNativeComponent: "RNSFullWindowOverlay" was not found in the UIManager
image

@kacperkapusciak
Copy link
Member

@jakob-p what Expo SDK version are you using?

Expo SDK 42 comes with react-native-screens v3.4.0.

FullWindowsOverlay is available from version >=3.7.0 of react-native-screens which will be available in Expo SDK 43.

Cheers

@jakob-p
Copy link

jakob-p commented Oct 19, 2021

Im using Expo SDK 42, but tried to upgrade react-native-screens manually to 3.8.0.

I'll try it with Expo SDK 43 when its officially released.

@kacperkapusciak
Copy link
Member

@jacobp100 you can't manually upgrade native dependencies in Expo's managed workflow. It bundles particular versions of 3rd party libraries into one SDK release.

See workflow comparison for reference

Cheers

@a-eid
Copy link

a-eid commented Oct 27, 2021

Note: this doesn't work with android yet.

  const Overlay = Platform.OS === 'ios' ? FullWindowOverlay : View;

falling back to a normal View works on android as well but only with RNS Modals, RN Modals covers the Overlay on android.

It works correctly on ios for Both RN Modals and RNS Modals

overlay2.mp4

@WoLewicki
Copy link
Member Author

Yeah, I don't think there is a good way to implement something similar on Android, at least I don't have an idea how could it work under the hood. If you have any suggestions or libraries that do something similar, please link it here.

@a-eid
Copy link

a-eid commented Oct 27, 2021

@WoLewicki I'm not really sure, I'll do some research my self for sure.
I am wondering what's the difference between RN Modal & RNS Screen Modal.

@nandorojo
Copy link

The key difference is that RNS lets you open more than one at a time, in order. RN modal is restricted to only one.

@a-eid
Copy link

a-eid commented Oct 27, 2021

@nandorojo that is the case on IOS ineed ( which is considered a bug ) , on android you can push multiple RN modals just fine.

@nandorojo
Copy link

If that's the case, then the normal modal should work fine on Android. I thought that it didn't.

It would be useful to have a component that falls back to Modal on non-iOS and matches the same behavior of the RNS overlay view (no animation, etc) so that we don't need to use Platform.OS in apps themselves.

@a-eid
Copy link

a-eid commented Oct 27, 2021

I don't think that would work

  • Modals appear on top of each other, so pushing another modal would still be above the RNS Overlay.
  • Modals cover the whole screen so they wouldn't allow touches to pass. >> pointerEvents="box-none" don't work on Modals.

@a-eid
Copy link

a-eid commented Oct 27, 2021

I think for android replacing the RN Modal with an Actual Portal View might solve the issue,
might need to check gorhom or react-native-paper Portal package.

@a-eid
Copy link

a-eid commented Oct 27, 2021

it's worth noting that on IOS the Overlay component does not work with RN modal with `presentationStyle="fullScreen".

somehow it removes it completely from the tree.

issue-ios-fullscreen.mp4

@WoLewicki
Copy link
Member Author

This behavior is a consequence of how didMoveToWindow is handled in this component. I made a draft PR that changes this behavior, but it also introduces other ones, which might not be wanted: https://github.com/software-mansion/react-native-screens/pull/1191/files. Please check if it is something you would like to use.

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.

Display any view on top of native-stack's modal
7 participants