Inset the full screen modal used for workspaces on large devices #3887

HorusGoul · 2021-07-06T15:28:12Z

cc @marcaaron

Details

Added the expected spacing around the full-screen modals without modifying too many things.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/169135

Tests / QA Steps

Using a large screen (like the web version or the desktop app):

Create a new workspace
Go to its settings page
Verify that the full-screen modal is separated from the window border and that its corners are rounded, like this photo:

Using a small screen (mobile version):

Create a new workspace
Go to its settings page
Verify that the full-screen modal is not separated from the window border and has no rounded corners, like in this photo:

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

HorusGoul · 2021-07-06T15:49:39Z

Should we set the overlay background color to the one used in the Figma design or leave it as rgba(0, 0, 0, 0.5)?

shawnborton · 2021-07-06T15:50:27Z

I think we should already have a color variable for the correct overlay color. If you launch an attachment modal from a chat, you should see what I mean.

HorusGoul · 2021-07-06T16:18:34Z

Done! I had to create a new component to customize the background color. Let me know what you think about the changes.

shawnborton · 2021-07-06T16:19:43Z

Mind sharing a screenshot?

marcaaron · 2021-07-06T18:49:42Z

I'm not sure if this is a blocker, but interacting with the backdrop won't dismiss this modal.

marcaaron

Mostly LGTM! But did have some questions

marcaaron · 2021-07-06T18:53:06Z

src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js

+
+    if (!props.isSmallScreenWidth) {
+        return (
+            <View style={styles.navigationSceneFullScreenWrapper}>


Could we change only the styles passed to sceneContainerStyle? Why is the additional View necessary? Asking mostly because it would make this logic a bit cleaner without having to create the content variable, but not a blocker.

That's what I tried first, and I found that the sceneContainerStyle targets a different UI component.

Adding the View wrapper was necessary because it was the only way to wrap the Drawer and the Screen contents and apply the rounded corners to them.

Got it. Does having the extra View around everything else cause issues as well? Something like

<View style={props.isSmallScreenWidth ? styles.navigationSceneFullScreenWrapper : undefined}> ... other stuff

Also no worries if there is no way to keep all the JSX together. I think it's just easier to parse without the content variable.

marcaaron · 2021-07-06T18:56:15Z

src/styles/styles.js

 * @param {Boolean} isSmallScreenWidth
 * @returns {Object}
 */
-function getNavigationDrawerStyle(windowWidth, windowHeight, isSmallScreenWidth) {
+function getNavigationDrawerStyle(isSmallScreenWidth) {


Out of curiosity, is this change just clean up or were the non relative width/height causing some issue?

Yep, the non-relative values were causing issues after I added the spacing around the modal (essentially, the modal wasn't respecting the parent's right and bottom padding), so I ended up making all the sizes relative to fix the issue.

An alternative to this was subtracting spacing size * 2 from the absolute values, but that was a worse solution IMO.

Yep, the non-relative values were causing issues after I added the spacing around the modal (essentially, the modal wasn't respecting the parent's right and bottom padding), so I ended up making all the sizes relative to fix the issue.

Got it, thanks. I think maybe in this case it would be good to add that context in a comment since a future change could undo that and the reasoning for these values is not very clear.

An alternative to this was subtracting spacing size * 2 from the absolute values, but that was a worse solution IMO.

That does sound more complicated.

marcaaron · 2021-07-06T19:11:48Z

src/components/CardOverlay.js

+            left: 0,
+            width: '100%',
+            height: '100%',
+            opacity: 0.5,


This style should be moved into styles.js. We have standardized on avoiding inline styles and there is some more explanation here.

HorusGoul · 2021-07-07T07:51:01Z

@shawnborton

Mind sharing a screenshot?

HorusGoul · 2021-07-07T10:25:28Z

@marcaaron

I'm not sure if this is a blocker, but interacting with the backdrop won't dismiss this modal.

We could use a Touchable inside the CardOverlay to handle these interactions. However, the visible surface of the overlay is tiny (20px per side), so it may not add that much value.

Have we considered adding support for the Esc key to close modals? I think that would be more suited for this use case.

marcaaron · 2021-07-07T17:16:26Z

We could use a Touchable inside the CardOverlay to handle these interactions. However, the visible surface of the overlay is tiny (20px per side), so it may not add that much value.

I'll defer to @shawnborton on this one. I see your point about the small surface, but also can see an argument where the universal expected behavior for a backdrop behind a modal (however small) would be to dismiss the modal. No strong feelings on it though :)

Have we considered adding support for the Esc key to close modals? I think that would be more suited for this use case.

esc key should already be enabled, yes

marcaaron

Code changes look good, but I think this PR is introducing a regression as the height expands much larger than the screen size. See video:

2021-07-07_07-14-21.mp4

marcaaron · 2021-07-07T17:18:13Z

src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js

+
+    if (!props.isSmallScreenWidth) {
+        return (
+            <View style={styles.navigationSceneFullScreenWrapper}>


Got it. Does having the extra View around everything else cause issues as well? Something like

<View style={props.isSmallScreenWidth ? styles.navigationSceneFullScreenWrapper : undefined}> ... other stuff

marcaaron · 2021-07-07T17:18:37Z

src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js

+
+    if (!props.isSmallScreenWidth) {
+        return (
+            <View style={styles.navigationSceneFullScreenWrapper}>


Also no worries if there is no way to keep all the JSX together. I think it's just easier to parse without the content variable.

marcaaron · 2021-07-07T17:27:41Z

src/styles/styles.js

 * @param {Boolean} isSmallScreenWidth
 * @returns {Object}
 */
-function getNavigationDrawerStyle(windowWidth, windowHeight, isSmallScreenWidth) {
+function getNavigationDrawerStyle(isSmallScreenWidth) {


Yep, the non-relative values were causing issues after I added the spacing around the modal (essentially, the modal wasn't respecting the parent's right and bottom padding), so I ended up making all the sizes relative to fix the issue.

Got it, thanks. I think maybe in this case it would be good to add that context in a comment since a future change could undo that and the reasoning for these values is not very clear.

An alternative to this was subtracting spacing size * 2 from the absolute values, but that was a worse solution IMO.

That does sound more complicated.

shawnborton · 2021-07-07T17:31:31Z

I don't feel strongly about the outside area being clickable. I agree with Marc though that it is expected behavior, but I could live without it here. I feel like the Esc key is more useful though so if I had to pick, I would want the Esc key before the outer area.

HorusGoul · 2021-07-09T12:26:12Z

I'll pick this back after coming back from OOO 😄

joelbettner

Left one comment. Other than that it LGTM.

src/styles/styles.js

joelbettner · 2021-07-16T22:18:24Z

src/libs/Navigation/AppNavigator/AuthScreens.js

@@ -215,10 +217,14 @@ class AuthScreens extends React.Component {
        };
        const fullscreenModalScreenOptions = {
            ...commonModalScreenOptions,
-            cardStyle: {...styles.fullscreenCard},
+            cardStyle: {
+                ...styles.fullscreenCard,


Since styles.cardOverlay is being added below and has the following styles:

cardOverlay: { backgroundColor: themeColors.modalBackdrop, position: 'absolute', top: 0, left: 0, width: '100%', height: '100%', opacity: 0.5, }

and styles.fullscreenCard has the following styles:

fullscreenCard: { position: 'absolute', left: 0, top: 0, width: '100%', height: '100%', }

why do we need to add ...styles.fullscreenCard here? Won't all these styles be included in cardOverlay?

The cardOverlay is for the backdrop of the modal, and the fullscreenCard is for the content that appears on top of the backdrop. These styles are for two different components, hence the need for the two different styles.

I was just wondering if the styles would be inherited. In any case, if the cardOverlay component might be used elsewhere, it makes sense to pass the styles along and not rely on inheritence.

joelbettner · 2021-07-16T22:18:32Z

src/libs/Navigation/AppNavigator/AuthScreens.js

            isFullScreenModal: true,
+            cardOverlay: CardOverlay,


Sould it make more sense just to have

cardOverlay: <View style={styles.cardOverlay} />,

Why have an entirely different file for something that is only used in this one place?

We could move it to the AuthScreen but, we would still need to pass it as cardOverlay: CardOverlay. I think it's best to leave it as a separate file just in case we need to use it for another modal.

…latest changes

HorusGoul · 2021-07-19T16:55:53Z

I just fixed the bug @marcaaron mentioned for the Reports screen. I still need to test other screens similar to the Reports one to see if there are similar regressions.

marcaaron · 2021-07-19T18:18:17Z

Let us know when it's ready for another review 👍

HorusGoul · 2021-07-20T11:03:13Z

Ready for review!

github-actions · 2021-07-20T11:03:29Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

HorusGoul · 2021-07-20T11:04:57Z

I have read the CLA Document and I hereby sign the CLA

marcaaron · 2021-08-02T19:06:14Z

I'm not sure if this PR was blocked on me (sorry, if so) but it has conflicts now.

HorusGoul · 2021-08-09T09:59:47Z

Conflicts fixed. If you could review @marcaaron

OSBotify · 2021-08-09T16:51:33Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-08-11T22:19:19Z

🚀 Deployed to staging by @marcaaron in version: 1.0.83-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-08-17T18:19:23Z

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

HorusGoul added 3 commits July 6, 2021 16:57

Inset the full screen modal used for workspaces on large devices

29761f4

Use style variables

8ad161a

Remove unused parameters from the getNavigationDrawerStyle function

67a6c14

HorusGoul self-assigned this Jul 6, 2021

HorusGoul requested a review from marcaaron July 6, 2021 15:48

HorusGoul marked this pull request as ready for review July 6, 2021 15:48

HorusGoul requested a review from a team as a code owner July 6, 2021 15:48

MelvinBot requested review from joelbettner and removed request for a team July 6, 2021 15:48

HorusGoul marked this pull request as draft July 6, 2021 15:53

HorusGoul added 2 commits July 6, 2021 17:55

Fix padding

208dd34

Add custom CardOverlay component so we can use the modalBackdrop color

ed7d06d

HorusGoul marked this pull request as ready for review July 6, 2021 16:18

marcaaron requested changes Jul 6, 2021

View reviewed changes

Move CardOverlay styles to the styles.js file

6315c42

HorusGoul requested a review from marcaaron July 7, 2021 10:25

marcaaron requested changes Jul 7, 2021

View reviewed changes

joelbettner suggested changes Jul 13, 2021

View reviewed changes

src/styles/styles.js Show resolved Hide resolved

joelbettner suggested changes Jul 16, 2021

View reviewed changes

Merge branch 'main' into horus-inset-full-screen-modal

684d5b2

Solve two style issues related to the reports screen provoked by the …

8de4c56

…latest changes

HorusGoul requested review from joelbettner and marcaaron July 20, 2021 11:00

joelbettner previously approved these changes Jul 20, 2021

View reviewed changes

Merge branch 'main' into horus-inset-full-screen-modal

703a063

HorusGoul dismissed joelbettner’s stale review via 703a063 August 9, 2021 09:58

joelbettner approved these changes Aug 9, 2021

View reviewed changes

marcaaron approved these changes Aug 9, 2021

View reviewed changes

marcaaron merged commit 21b1ee4 into main Aug 9, 2021

marcaaron deleted the horus-inset-full-screen-modal branch August 9, 2021 16:50

OSBotify mentioned this pull request Aug 11, 2021

Deploy Checklist: Expensify.cash 2021-08-11 #4554

Closed

53 tasks

Inset the full screen modal used for workspaces on large devices #3887

Inset the full screen modal used for workspaces on large devices #3887

Conversation

HorusGoul commented Jul 6, 2021 • edited Loading

Details

Fixed Issues

Tests / QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

HorusGoul commented Jul 6, 2021

shawnborton commented Jul 6, 2021

HorusGoul commented Jul 6, 2021

shawnborton commented Jul 6, 2021

marcaaron commented Jul 6, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HorusGoul commented Jul 7, 2021 • edited Loading

HorusGoul commented Jul 7, 2021

marcaaron commented Jul 7, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnborton commented Jul 7, 2021

HorusGoul commented Jul 9, 2021

joelbettner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HorusGoul commented Jul 19, 2021

marcaaron commented Jul 19, 2021

HorusGoul commented Jul 20, 2021

github-actions bot commented Jul 20, 2021 • edited Loading

HorusGoul commented Jul 20, 2021

marcaaron commented Aug 2, 2021

HorusGoul commented Aug 9, 2021

OSBotify commented Aug 9, 2021

OSBotify commented Aug 11, 2021

OSBotify commented Aug 17, 2021

HorusGoul commented Jul 6, 2021 •

edited

Loading

HorusGoul commented Jul 7, 2021 •

edited

Loading

github-actions bot commented Jul 20, 2021 •

edited

Loading