Update app modal design refactor #1973

SameeraMadushan · 2021-03-22T08:06:12Z

Details

Update modal design modified with adding a new modal type called CONFIRM.
Also HeaderWithCloseButton replaced with the Header component inside BaseUpdateAppModal

Fixed Issues

Fixes #1853

Tests

To view the update modal temporarily you have to set visibility to true. To do that you can do the following.

Set this.props.updateAvailable to !this.props.updateAvailable in Expensify.js
Otherwise, Wait for an update to be deployed, Observe the modal that appears.

Tested On

Screenshots

Web

Web - small screen

Mobile Web

Desktop

iOS

Android

….cash into sameera-update-app-modal-refactor

shawnborton · 2021-03-22T09:28:31Z

src/styles/getModalStyles.js

+
+                borderRadius: 12,
+                overflow: 'hidden',
+                width: isSmallScreenWidth ? '90%' : windowWidth * 0.3,


I think we want to have a fixed width on all non-small screens, so I think just using 375 (the same width as the sidebar) works.

For small screens, I think it makes sense to repeat the bottomDocked modal pattern here where it would look something like:

cc @roryabraham

@shawnborton Sure. I'll update the changes.

roryabraham

I think of a "confirm" modal as having modal which is sized like the one you have here, but which also:

Does not have an x to close in the header
Has two buttons, one "confirm" button and one "cancel", both of which should close the modal

This is a very common reusable pattern for a modal, so I'd like to see it created as it's own component. You could call it ConfirmModal, and give it props like this:

const propTypes = {
    title: PropTypes.string.isRequired,
    onConfirm: PropTypes.func.isRequired,
    onCancel: PropTypes.func.isRequired,
    confirmText: PropTypes.string,
    cancelText: PropTypes.string,
    prompt: PropTypes.string,
};

const defaultProps = {
    confirmText: 'Yes',
    cancelText: 'No',
    prompt: '',
};

That component would internally use withWindowDimensions, and be BottomDocked on small screens and confirm on wider screens. Then BaseUpdateAppModal can use the ConfirmModal.

roryabraham · 2021-03-22T16:46:43Z

src/components/UpdateAppModal/BaseUpdateAppModal.js

@@ -33,31 +35,42 @@ class BaseUpdateAppModal extends PureComponent {
                    onSubmit={this.submitAndClose}
                    onClose={() => this.setState({isModalOpen: false})}
                    isVisible={this.state.isModalOpen}
+                    type={this.props.isSmallScreenWidth
+                        ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED : CONST.MODAL.MODAL_TYPE.CONFIRM}


NAB typically we format multiline ternaries like this:

const result = condition ? valueIfTrue : valueIfFalse;

I'll update it.

roryabraham · 2021-03-22T16:47:13Z

src/components/UpdateAppModal/BaseUpdateAppModal.js

-                        Update now or restart the app at a later time to download the latest changes.
-                    </Text>
-                    {this.props.onSubmit && (
+                    <View style={[styles.m5]}>


No need for square brackets if it's just a single style.

Suggested change

<View style={[styles.m5]}>

<View style={styles.m5}>

roryabraham · 2021-03-22T16:47:21Z

src/components/UpdateAppModal/BaseUpdateAppModal.js

-                    </Text>
-                    {this.props.onSubmit && (
+                    <View style={[styles.m5]}>
+                        <View style={[styles.flexRow]}>


Suggested change

<View style={[styles.flexRow]}>

<View style={styles.flexRow}>

Why does the header need to be wrapped in a view w/ flexRow ?

Because <Header> component does have another view and it has flex1.
In other places <Header> component is used inside a <view> with flexRow.
If I remove flexRow from here it won't show in the mobile app.

Ok, thanks for explaining.

SameeraMadushan · 2021-03-22T18:06:28Z

I think of a "confirm" modal as having modal which is sized like the one you have here, but which also:

Does not have an x to close in the header

Has two buttons, one "confirm" button and one "cancel", both of which should close the modal

This is a very common reusable pattern for a modal, so I'd like to see it created as it's own component. You could call it ConfirmModal, and give it props like this:
const propTypes = {
    title: PropTypes.string.isRequired,
    onConfirm: PropTypes.func.isRequired,
    onCancel: PropTypes.func.isRequired,
    confirmText: PropTypes.string,
    cancelText: PropTypes.string,
    prompt: PropTypes.string,
};

const defaultProps = {
    confirmText: 'Yes',
    cancelText: 'No',
    prompt: '',
};
That component would internally use withWindowDimensions, and be BottomDocked on small screens and confirm on wider screens. Then BaseUpdateAppModal can use the ConfirmModal.

I will do it accordingly.

But I have few questions.

The modal visibility will be handled inside the ConfirmModal. Is that ok?
Do we need to have onCancel prop? Because we need onCancel only to set the visibility to false right? We can do it inside the ConfirmModal

roryabraham · 2021-03-22T18:51:39Z

The modal visibility will be handled inside the ConfirmModal. Is that ok?

Hmmm, this is a bit contrary to how our other modals typically work. We should give ConfirmModal an isVisible prop, and let the parent component manage the visibility via the onConfirm and onCancel props. Sorry I missed the isVisible boolean prop in my change request above.

roryabraham

Overall looks good, just some minor code style comments. Also, since you just have one file in the ConfirmModal module, you don't need it to be a directory. You can just move src/components/ConfirmModal/index.js to src/components/ConfirmModal.js

src/components/ConfirmModal/index.js

roryabraham

Nice, thanks for the quick turnaround. Code looks good. I tested it out a bit too and it seems to work well. However, can you please be sure to retest on all platforms and provide screenshots for @shawnborton to re-review. In my opinion, the font size for the prompt maybe looks a bit small and has maybe too large a top margin, but otherwise all looks good.

SameeraMadushan · 2021-03-22T20:22:50Z

Thanks @roryabraham
I will retest and upload new screenshots for review.

SameeraMadushan · 2021-03-22T20:50:25Z

New screenshots with PR changes

Web

Mobile Web

Desktop

iOS

Android

shawnborton · 2021-03-23T08:51:21Z

Could you please update the screenshots in the original comment of this PR?

SameeraMadushan · 2021-03-23T09:02:20Z

@shawnborton PR comment updated with latest screenshots.

shawnborton · 2021-03-23T11:11:18Z

src/components/ConfirmModal.js

+                <Header title={props.title} />
+            </View>
+
+            <Text style={[styles.textLabel, styles.mt4]}>


Can we use styles.textP here please?

Also, this is not a blocker but I think I prefer adding margin to the bottom of the modal header as opposed to the top of this text block.

shawnborton · 2021-03-23T11:14:23Z

src/components/ConfirmModal.js

+                    style={[
+                        styles.buttonText,
+                        styles.buttonSuccessText,
+                        styles.buttonConfirmText,


What does buttonConfirmText do versus buttonSuccessText? I'm not entirely sure why buttonConfirmText exists, it looks like it just adds padding to the left and the right but I actually don't think we need it. I would suggest we remove this.

Yes. styles.buttonConfirmText is for add horizontal padding. Seems we don't need it here. I'll remove it.

Awesome, thank you! Changes look good, you'll just need to update the screenshots again.

Thanks! Screenshots updated.

Jag96

On Web when I load the app up, I see that the Cancel button is highlighted with a blue border, is that expected or is there something we can do to remove that?

Other than that, looks great!

roryabraham · 2021-03-23T18:24:55Z

Interestingly, I noticed the same thing but didn't comment because the view component doesn't support the focusable prop in react-native-web 14. @SameeraMadushan If you wanted, however, you could update react-native-web to v15, which now supports that prop. https://github.com/necolas/react-native-web/releases/tag/0.15.0

SameeraMadushan · 2021-03-23T18:37:53Z

Thanks for the suggestion @roryabraham. Setting focusable to false fixed the issue.
I will do the testing and push changes with react-native-web v15.

roryabraham · 2021-03-23T18:48:31Z

Hooray! ....hopefully we don't break anything 😬

roryabraham · 2021-03-23T18:53:41Z

We'll also need to upgrade to react 17

SameeraMadushan · 2021-03-23T18:56:09Z

We'll also need to upgrade to react 17

Tested with all platforms. It works without any regression.
Is it good to upgrade react independently?

roryabraham · 2021-03-23T18:59:39Z

Well, I just noticed that react 17 is listed as a peer dependency, which makes me more nervous to greenlight this change.

Jag96 · 2021-03-23T19:00:27Z

Thanks for looking into that! Since it's already a pre-existing issue with our app and it requires updating some libs that would affect all functionality, I suggest we do this in a different PR.

@SameeraMadushan can we revert the update to react-native-web and hold off on updating react to 17 in this PR? We can create a new issue for the blue border and handle it separately.

SameeraMadushan · 2021-03-23T19:05:22Z

@roryabraham I see react 17 used in react native 0.64. So it's a bit hard to predict what will happen.
upgrade helper
@Jag96 Can do that.

roryabraham · 2021-03-23T19:05:47Z

Yeah, sorry for the go-around here @SameeraMadushan, let's tackle this later.

This reverts commit f16ff72.

isagoico · 2021-03-25T21:41:16Z

@SameeraMadushan Hello! This PR looks like internal QA. If so, can you let me know when it's QA'd in staging so I can check it off the deploy list? Thanks in advance!

roryabraham · 2021-03-25T21:45:46Z

@isagoico, in this case there's really no easy way to QA the issue on staging, so feel free to check it off the deploy list.

SameeraMadushan added 2 commits March 22, 2021 13:20

Update modal design change

8cf7fe5

Merge branch 'master' of https://github.com/SameeraMadushan/Expensify…

8022874

….cash into sameera-update-app-modal-refactor

SameeraMadushan requested a review from a team as a code owner March 22, 2021 08:06

botify requested review from Jag96 and removed request for a team March 22, 2021 08:06

shawnborton reviewed Mar 22, 2021

View reviewed changes

Adding bottom docked modal and fixing width

7a2d5a5

roryabraham self-requested a review March 22, 2021 16:44

roryabraham assigned SameeraMadushan Mar 22, 2021

roryabraham requested changes Mar 22, 2021

View reviewed changes

Re-usable confirm modal added

c264880

roryabraham requested changes Mar 22, 2021

View reviewed changes

Fixing Review comments

3ad5627

roryabraham previously approved these changes Mar 22, 2021

View reviewed changes

roryabraham requested a review from shawnborton March 22, 2021 20:58

shawnborton reviewed Mar 23, 2021

View reviewed changes

Style changes for Review

9adb0a5

SameeraMadushan dismissed roryabraham’s stale review via 9adb0a5 March 23, 2021 11:54

shawnborton previously approved these changes Mar 23, 2021

View reviewed changes

Jag96 reviewed Mar 23, 2021

View reviewed changes

Updating react-native-web to v15

f16ff72

Setting touchable focusable to false

a2bd79b

SameeraMadushan dismissed shawnborton’s stale review via a2bd79b March 23, 2021 18:54

Revert "Updating react-native-web to v15"

210f03d

This reverts commit f16ff72.

roryabraham approved these changes Mar 23, 2021

View reviewed changes

Jag96 approved these changes Mar 23, 2021

View reviewed changes

Jag96 merged commit c2571b7 into Expensify:master Mar 23, 2021

OSBotify mentioned this pull request Mar 23, 2021

Deploy Checklist: Expensify.cash 2021-03-19 #1926

Closed

85 tasks

	<View style={[styles.flexRow]}>
	<View style={styles.flexRow}>

Update app modal design refactor #1973

Update app modal design refactor #1973

Conversation

SameeraMadushan commented Mar 22, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Web - small screen

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham 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

SameeraMadushan Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SameeraMadushan commented Mar 22, 2021

roryabraham commented Mar 22, 2021 • edited Loading

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

SameeraMadushan commented Mar 22, 2021

SameeraMadushan commented Mar 22, 2021

New screenshots with PR changes

Web

Mobile Web

Desktop

iOS

Android

shawnborton commented Mar 23, 2021

SameeraMadushan commented Mar 23, 2021

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

Jag96 left a comment

Choose a reason for hiding this comment

roryabraham commented Mar 23, 2021

SameeraMadushan commented Mar 23, 2021

roryabraham commented Mar 23, 2021

roryabraham commented Mar 23, 2021

SameeraMadushan commented Mar 23, 2021

roryabraham commented Mar 23, 2021

Jag96 commented Mar 23, 2021

SameeraMadushan commented Mar 23, 2021

roryabraham commented Mar 23, 2021

isagoico commented Mar 25, 2021

roryabraham commented Mar 25, 2021

SameeraMadushan commented Mar 22, 2021 •

edited

Loading

SameeraMadushan Mar 22, 2021 •

edited

Loading

roryabraham commented Mar 22, 2021 •

edited

Loading

roryabraham left a comment •

edited

Loading