Create Right-Docked Settings Modal by yuwenmemon · Pull Request #1325 · Expensify/App

yuwenmemon · 2021-01-25T22:09:00Z

@marcaaron please review

Details

Create a right-docked settings modal.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/150956

Tests

For each of the formats below, click on the user icon to bring up the settings modal.
X out of the settings modal, make sure that you are brought back to the report you were viewing before.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2021-01-25T22:09:16Z

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

marcaaron

Looks great so far! Just leaving early feedback since it's still in WIP.

marcaaron · 2021-01-25T22:37:19Z

 class MainView extends Component {
    render() {
-        const reportIDInUrl = parseInt(this.props.match.params.reportID, 10);
+        let activeReportID = parseInt(this.props.match.params.reportID, 10);


I'm curious what would happen if we simply stopped relying on the match params here. IIRC we should be updating the currentlyViewedReportID when we hit one of these routes.

By that you mean just doing

activeReportID = parseInt(this.props.currentlyViewedReportID, 10);

always, right?

Yeah exactly. I'm trying to think of a time when the reportID in the URL wouldn't be the currentlyViewedReportID and not sure there is one. If we don't need it then the logic in this file becomes a little simpler and we can stop making an exception for the settings route.

marcaaron · 2021-01-25T22:38:23Z

+        case CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED:
+            modalStyle = {
+                margin: 0,
+                flexDirection: 'row-reverse',


Is this doing anything? I think it would be flex-end that would move the modal to the right of the screen.

I was following the flex-direction example here: https://reactnative.dev/docs/flexbox#flex-direction

If I remove this then the modal goes to the left side.

Cool. I guess it works then? If your width was 100% of the screen then I'd think flex-end would stick it at the right side but didn't really dig too much into what this is doing

I think I see what you're saying. Switched this up to use row and end below.

marcaaron · 2021-01-25T22:40:54Z

            break;
+        case CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED:
+            modalStyle = {
+                margin: 0,


NAB, pretty much every modal has this margin: 0 on it. I wonder if this should just be a default for all of them.

marcaaron · 2021-01-25T22:43:53Z

+                justifyContent: 'end',
+            };
+            modalContainerStyle = {
+                width: isSmallScreen ? '100%' : '40%',


cc @shawnborton we might want to use a constant for the right side pane instead of the 40%? I know there wasn't really a clear guideline for this part.

Yeah maybe a max-width? 450px maybe?

What if we just reuse the variable for the LHN? So basically at mobile breakpoints, this should take up the whole screen (like the LHN) but at wider desktops, this should share the same LHN variable which would make it 375px. This is what we've been doing in all of our mockups.

@yuwenmemon I think you'll need to pull master, and then you should find sideBarWidth: 375 inside of variables.js

marcaaron · 2021-01-25T22:45:11Z

+                height: '100%',
+                overflow: 'hidden',
+                marginRight: 0,
+                paddingTop: 0,


NAB, I haven't looked yet but who/what is adding margin and padding that we need to reset it? Maybe it would be better to only add margin/padding to the places that need it instead of override it here.

marcaaron · 2021-01-25T22:46:00Z

+                >
+                    <View>
+                        <SettingsPage />
+                    </View>


Do we need this extra View?

yuwenmemon · 2021-01-26T03:04:03Z

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

yuwenmemon · 2021-01-26T03:04:23Z

recheck

yuwenmemon · 2021-01-26T05:11:11Z

Removing WIP

shawnborton · 2021-01-26T09:32:23Z

One small thing I found (maybe not related to this PR at all) is that when you launch the settings sidepane modal, the active style of the LHN row, where the background is a darker color, goes away:

Settings open:

Settings closed:

marcaaron · 2021-01-26T16:53:58Z

@shawnborton Interesting, is it disappearing or is it just the same color as the overlay?

shawnborton · 2021-01-26T17:00:06Z

It's actually disappearing, the property gets removed.

marcaaron · 2021-01-26T16:58:44Z

+    currentlyViewedReportID: '',
+};
+
+class SettingsModal extends Component {


NAB, we can probably make this a stateless functional component by using an anonymous arrow function for the onClose method.

marcaaron · 2021-01-26T16:59:46Z

+                    </Text>
+                </TouchableOpacity>
            </View>
+


NAB, new line

marcaaron · 2021-01-26T17:01:01Z

                                    />
-                                    {this.props.currentURL === '/settings' && <SettingsPage />}
+                                    <SettingsModal
+                                        isVisible={this.props.currentURL === '/settings'}


NAB, this was left by someone else but can we take this opportunity to update /settings to use the constant in ROUTES?

marcaaron · 2021-01-26T17:02:05Z

 class MainView extends Component {
    render() {
-        const reportIDInUrl = parseInt(this.props.match.params.reportID, 10);
+        let activeReportID = parseInt(this.props.match.params.reportID, 10);


Yeah exactly. I'm trying to think of a time when the reportID in the URL wouldn't be the currentlyViewedReportID and not sure there is one. If we don't need it then the logic in this file becomes a little simpler and we can stop making an exception for the settings route.

marcaaron · 2021-01-26T17:08:11Z

Tests great and looks real nice so far. Left some NABs. And I'm still curious if we can just ditch the match params for reportIDInUrl. Also noticed that the header and LHN row are disappearing in some cases (probably because they are also relying on the reportIDInUrl)

2021-01-26_07-06-41.mp4

shawnborton · 2021-01-26T22:31:58Z

Signing off for the night, so don't block on me if you want to merge this today. I think we just need to update the screenshots and make sure the side modal on desktop is 375px wide. Nice work @yuwenmemon !

…wen-settingsModal

marcaaron

Looks good two more small comments.

marcaaron · 2021-01-26T22:51:37Z

+const SettingsModal = props => (
+    <ModalWithHeader
+        type={CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}
+        onClose={() => redirect(ROUTES.getReportRoute(props.currentlyViewedReportID))}


This seems like a minor regression from what we were doing before. Which is, cover the case where currentlyViewedReportID has no value.

Ah yep, missed that - nice catch.

marcaaron · 2021-01-26T22:57:32Z

            // viewed on a smaller device (e.g. mobile or mobile web).
-            modalStyle = {
-                margin: 0,
+            modalStyle = _.extend(modalStyle, {


NAB, I believe we have more or less standardized on using spread operator instead of _.extend but could be wrong about that.

Actually yes double checked, there are 0 usages in the codebase. For consistency, maybe we can stick to just using spread operator? Mostly my concern is that we will have 2 ways of doing one thing. Just like we shouldn't be mixing Array.map and _.map we shouldn't be using {...something} and _.extend(something, {});

yuwenmemon · 2021-01-27T00:24:03Z

Updated, and updated the Web GIF to be more up to date

marcaaron · 2021-01-27T00:44:08Z

        : [styles.sidebarHeader];

-    const {recentReports} = getSidebarOptions(props.reports, props.personalDetails, props.draftComments, reportIDInUrl);
+    const {recentReports} = getSidebarOptions(props.reports, props.personalDetails, props.draftComments, activeReportID);


We should fix this. It's a warning but we are treating them as not optional. I have an open issue to make this more clear.

Ah, sorry didn't realize what you were referring to at first. You're referring to the fact that this is a linter warning.

marcaaron · 2021-01-27T00:46:43Z

This is looking good to me and testing well. However, the out animation looks a little fast on web (almost like it's not happening at all?). also, the mobile web video seems to just be web at a mobile resolution. We should make sure we are testing mobile web by using something like iOS simulator + Safari.

marcaaron · 2021-01-27T01:03:40Z

Looks like the animations for the side bar are not happening on Android. iOS looks ok though.

yuwenmemon · 2021-01-27T01:58:53Z

This is looking good to me and testing well. However, the out animation looks a little fast on web (almost like it's not happening at all?)

So, it's odd - for whatever reason on browsers the out animation is faster than the in (was the same case in Safari as Chrome).
In: 300, Out: 900

But then doing that makes it super slow on mobile:

marcaaron · 2021-01-27T02:02:59Z

Oh hmm, that is super weird. Maybe it's a bug within the modal library. Either way we don't necessarily have to worry about it now.

marcaaron · 2021-01-27T02:04:46Z

Once work-around could be to set a different animationOutTiming value for web... but ideally we could figure out why they are different.

yuwenmemon · 2021-01-27T02:18:54Z

the mobile web video seems to just be web at a mobile resolution. We should make sure we are testing mobile web by using something like iOS simulator + Safari.

Done - updated OP

Looks like the animations for the side bar are not happening on Android.

~~Yeah... I have no idea why this is happening... 😕 In general, my android emulator is pretty laggy so it's been hard to see anyway.~~

EDIT: Okay, from what I can gather, looks like android suffers from the same inconsistency that web does vs iOS. Lowering the animation speeds brought the animation back. Seems like android likes to slow down a bunch at the end, hence why at the faster speeds it looks like it does not have any animation
In 2000 / Out 2000

yuwenmemon · 2021-01-27T18:47:09Z

@marcaaron do we want to worry about the modal animation timing inconsistency across platforms for now or not?

marcaaron · 2021-01-27T20:19:09Z

It would be nice to know why if we can look into it now. But it's up to you. I'm also fine with creating a new issue to follow up.

yuwenmemon · 2021-01-27T23:04:49Z

@marcaaron dug around for an hour or so, didn't really get anywhere so I vote we move forward and just create a new issue to follow up.

marcaaron · 2021-01-27T23:13:58Z

Ok I agree. One follow-up idea is to see if this is just generally reproducible and then create an issue in react-native-modal. It would be good to know if we are holding it wrong or if there is a bug. Then we can add that context to the issue.

Separately, there are a couple of unresolved comments from me here. Other than that I am 👍

yuwenmemon · 2021-01-27T23:25:42Z

Yeah, I'd also want to try it on real devices as opposed to simulators.

Also, I believe there was just the one comment about style. Sorry, didn't understand what you were saying at first. But I'm not sure there are any others? If you could point them out that would be helpful.

marcaaron · 2021-01-27T23:48:37Z

Looks great thanks!

shawnborton · 2021-01-28T10:22:26Z

Looks great! I agree, let's please do a follow up issue for finessing the animation style.

yuwenmemon added 3 commits January 22, 2021 17:16

Get settings modal popping in and out from the right-side of the screen

c13f0cf

Cleanup style for the right-docked modal header

4c3915f

Remember current report and slide in the right direction

122ee3f

yuwenmemon requested a review from marcaaron January 25, 2021 22:09

yuwenmemon requested a review from a team as a code owner January 25, 2021 22:09

yuwenmemon self-assigned this Jan 25, 2021

botify requested review from madmax330 and removed request for a team January 25, 2021 22:09

marcaaron reviewed Jan 25, 2021

View reviewed changes

yuwenmemon added 3 commits January 25, 2021 15:18

Get rid unneccessary Views

f67c266

Get the version number showing at the very bottom

38a1934

Cleanup code / add some comments

04f0466

yuwenmemon changed the title ~~[WIP] Create Right-Docked Settings Modal~~ Create Right-Docked Settings Modal Jan 26, 2021

Update styles for better padding on mobile

c929ff3

marcaaron requested changes Jan 26, 2021

View reviewed changes

Fix conflicts

c898d00

yuwenmemon force-pushed the yuwen-settingsModal branch from bfe5f14 to 2223202 Compare January 26, 2021 21:36

PR comments

6039761

yuwenmemon force-pushed the yuwen-settingsModal branch from 2223202 to 6039761 Compare January 26, 2021 21:37

yuwenmemon commented Jan 26, 2021

View reviewed changes

Comment thread src/pages/home/HeaderView.js

shawnborton previously approved these changes Jan 26, 2021

View reviewed changes

yuwenmemon added 2 commits January 26, 2021 14:40

JS linting

6f20d8e

Merge branch 'master' of github.com:Expensify/ReactNativeChat into yu…

3ee27bd

…wen-settingsModal

yuwenmemon dismissed shawnborton’s stale review via 6f20d8e January 26, 2021 22:41

yuwenmemon force-pushed the yuwen-settingsModal branch from d98a40f to 6f20d8e Compare January 26, 2021 22:41

marcaaron requested changes Jan 26, 2021

View reviewed changes

Account for case where currentlyViewedReportID is empty

39e7069

use spread operator instead of extend

91dff12

marcaaron reviewed Jan 27, 2021

View reviewed changes

Fix style of function call

167d96d

marcaaron approved these changes Jan 27, 2021

View reviewed changes

yuwenmemon merged commit ea7a327 into master Jan 27, 2021

yuwenmemon deleted the yuwen-settingsModal branch January 27, 2021 23:54

github-actions Bot locked and limited conversation to collaborators Jan 27, 2021

Conversation

yuwenmemon commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

github-actions Bot commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwenmemon Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwenmemon commented Jan 26, 2021

Uh oh!

yuwenmemon commented Jan 26, 2021

Uh oh!

yuwenmemon commented Jan 26, 2021

Uh oh!

shawnborton commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron commented Jan 26, 2021

Uh oh!

shawnborton commented Jan 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Jan 26, 2021

Uh oh!

Uh oh!

shawnborton commented Jan 26, 2021

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

yuwenmemon commented Jan 25, 2021 •

edited

Loading

github-actions Bot commented Jan 25, 2021 •

edited

Loading

yuwenmemon Jan 26, 2021 •

edited

Loading

shawnborton commented Jan 26, 2021 •

edited

Loading

marcaaron Jan 26, 2021 •

edited

Loading

yuwenmemon commented Jan 27, 2021 •

edited

Loading

yuwenmemon commented Jan 27, 2021 •

edited

Loading