Remove close button from HeaderWithCloseButton by staszekscp · Pull Request #17963 · Expensify/App

staszekscp · 2023-04-25T11:24:03Z

Details

Removes close button from HeaderWithCloseButton component - expained here

Fixed Issues

$ #15850

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

* test flatlist for android * fix linter * fix name * fix not working features * limit rerenders

MelvinBot · 2023-04-25T11:24:38Z

@puneetlath Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

puneetlath

This mostly looks good to me. Just a few things:

you have lint errors
I don't think we should pass the onBackButtonPress prop if we're just setting it to Navigation.goBack since that is the default
Let's make sure that we do the all-device testing and include videos since this touches so many components

cc @mountiny in case you want to review as well.

puneetlath · 2023-04-25T15:06:20Z

src/components/HeaderWithBackButton.js

    onDownloadButtonPress: () => {},
-    onCloseButtonPress: () => {},
-    onBackButtonPress: () => {},
+    onBackButtonPress: Navigation.goBack,


I notice that on a lot of components we set this prop to Navigation.goBack, but given that is the default, why even pass this prop at all?

Agree. We should remove all redundant code.

In some cases the onBackButtonPress should bring user back to a previous step in a flow (some screens are implemented as "steps", therefore navigation is not used)

We can override this prop on those cases, but no need on normal cases as Navigation.goBack is already set as default value.
Your current branch has double Navigation.goBack for most screens. I know this is already existing code but we can remove all of them since Navigation.goBack is now set as default prop.

puneetlath · 2023-04-25T15:10:34Z

@mountiny perhaps we want to get a C+ to test this PR as well?

mountiny · 2023-04-25T18:23:05Z

Thanks @puneetlath

Lets fix the lint issues and we can run the internal release which will help with testing. this should mainly remove the X button from all the RHP pages so there should not be a big problem hopefully

mountiny

Thanks! Asking for a C+

mountiny · 2023-04-25T18:29:25Z

src/components/HeaderWithBackButton.js

-    onBackButtonPress: () => {},
+    onBackButtonPress: Navigation.goBack,
    onThreeDotsButtonPress: () => {},
-    shouldShowBackButton: false,


Was there any place where the shouldShowBackButton was false? are we covering that case?

This cannot be removed. i.e. ReportScreen have conditional back button render based on this prop.

There we go

0xmiroslav · 2023-04-25T18:31:22Z

@staszekscp please run npm run lint and fix all lint errors like this:
'reportID' is assigned a value but never used

0xmiroslav · 2023-04-25T18:39:08Z

src/components/HeaderWithBackButton.js

    onThreeDotsButtonPress: () => {},
-    shouldShowBackButton: false,


This cannot be removed. i.e. ReportScreen have conditional back button render based on this prop.

0xmiroslav · 2023-04-25T18:39:52Z

src/components/HeaderWithBackButton.js

    onDownloadButtonPress: () => {},
-    onCloseButtonPress: () => {},
-    onBackButtonPress: () => {},
+    onBackButtonPress: Navigation.goBack,


Agree. We should remove all redundant code.

0xmiroslav · 2023-04-25T18:41:15Z

src/components/ReimbursementAccountLoadingIndicator.js

+        <HeaderWithBackButton
            title={props.translate('reimbursementAccountLoadingAnimation.oneMoment')}
-            onCloseButtonPress={Navigation.dismissModal}
-            shouldShowBackButton={props.network.isOffline}


Doesn't this cause regression? I think it was intentional to set props.network.isOffline

Thanks for the feedback! I've noticed that if we remove X icon from every modal we won't have a chance to dismiss/go back and users would get stuck... That's why I decided to remove the shouldShowBackButton prop.

We can provide it conditionally (the default could be true to avoid passing the prop to almost every component), but then there should be some kind of mechanism to allow users to go back. Maybe we could leave conditional X button?

That's why I decided to remove the shouldShowBackButton prop

Report screen doesn't need back button on web but on mobile, so this prop is necessary

And it seems to be working fine with this implementation - the back button is only visible on small screen (on wide web there is no back button as expected)

RHP modal screens still need back button even on web

On the small screen the button is visible

Actually ReportScreen uses another component called HeaderView to render the back button, so this PR shouldn't affect it at all

So you still suggest to remove shouldShowBackButton prop completely and always show back arrow right?
I will check and confirm if any other pages need this button hidden.

I think we are fine to deprecate shouldShowBackButton prop and always show back button because close button is no longer visible.
i.e. Settings page had close button but now that it's removed, no way to close this on web. Clicking outside is the only way.

You can also click the UP/Back button to close it so we are good here!

mountiny · 2023-04-26T18:12:48Z

Thanks for the discussion so far, running an internal build so this is a bit easier to test https://github.com/Expensify/App/actions/runs/4811973135

OSBotify · 2023-04-26T19:08:09Z

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/17963/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/17963/index.html

desktop 💻	web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/17963/NewExpensify.dmg	https://17963.pr-testing.expensify.com

mountiny · 2023-04-26T21:12:34Z

Tested the build on web and on my ihone and it looks great ot what I can see

There is this skeleton issue with LHN on iOS for me
, not sure if this issue will be fixing it #17962

0xmiroslav

Request money pages still have close button

0xmiroslav · 2023-04-26T21:24:43Z

src/components/ReimbursementAccountLoadingIndicator.js

+        <HeaderWithBackButton
            title={props.translate('reimbursementAccountLoadingAnimation.oneMoment')}
-            onCloseButtonPress={Navigation.dismissModal}
-            shouldShowBackButton={props.network.isOffline}


I think we are fine to deprecate shouldShowBackButton prop and always show back button because close button is no longer visible.
i.e. Settings page had close button but now that it's removed, no way to close this on web. Clicking outside is the only way.

0xmiroslav · 2023-04-26T21:28:45Z

Is it expected to show back button and deprecate close button even on this kind of modal?

0xmiroslav · 2023-04-26T21:34:13Z

Not related to this PR but this regression is caused by navigation refactor and should be fixed:
Drop to upload feature broken
Let's create a follow-up issue for this so we don't miss it.

Screen.Recording.2023-04-26.at.11.32.00.PM.mov

mountiny · 2023-04-26T22:08:48Z

Is it expected to show back button and deprecate close button even on this kind of modal?

Oh good catch, I think the attachement modal on desktop should have X it makes more sense than <

but mobile would still have the <

0xmiroslav · 2023-04-27T04:27:16Z

@staszekscp please fix merge conflict.

Also, I think we should re-think about shouldShowBackButton prop.

Is it expected to show back button and deprecate close button even on this kind of modal?

Oh good catch, I think the attachement modal on desktop should have X it makes more sense than <

but mobile would still have the <

Based on this, we may still need both shouldShowBackButton and shouldShowCloseButton prop.

mountiny · 2023-04-27T04:30:13Z

@0xmiroslav I would say if the attachment modal is the only place we found, I think we dont need that and we could make some custom wrapper fro that header. I think eventually we will drop usage of that modal and the gallery for pictures will be more common to what you are used to from whatsapp or messenger.

0xmiroslav · 2023-04-27T04:31:46Z

@0xmiroslav I would say if the attachment modal is the only place we found, I think we dont need that and we could make some custom wrapper fro that header. I think eventually we will drop usage of that modal and the gallery for pictures will be more common to what you are used to from whatsapp or messenger.

Send attachment design will also change?

mountiny · 2023-04-27T04:42:32Z

I would imagine, this is not being worked on yet. But yeah, we need to keep it for now so whatever is easier now, keep the props and use it only in the attachment modal or something else. We need it there.

btw created an issue here #18064

staszekscp · 2023-04-27T07:49:40Z

There are actually two modals where the X button could be applied. I think we could add a prop showCloseButton and display it conditionally depending on screen size there. Let me do that tweak quickly

staszekscp · 2023-04-27T08:05:01Z

You may have a look now after some cleanup and fixes :)

mountiny

This is looking good to me

I have noticed this one little bug on pronouns page and create issue for it so we dont forget #18092 lets not fix it here

@0xmiroslav would you be able to go through the checklist here please? thank you!

0xmiroslav

I see you changed FlatList/index.android.js. Is this relevant to this issue?

0xmiroslav · 2023-04-28T00:57:54Z

src/components/HeaderWithBackButton.js

+    /** Whether we should show a close button */
+    shouldShowCloseButton: PropTypes.bool,
+
+    /** Method to trigger when pressing close button of the header */
+    onCloseButtonPress: PropTypes.func,
+


Please put this in original order for no code diff

Can we deprecate onCloseButtonPress and use onBackButtonPress always as there's no case of both back button and close button show

There's an edge case in two modals, so I decided to keep it for the web

0xmiroslav

These change original behavior. As they were added intentionally, let's make sure that there aren't any regressions after these removals.

0xmiroslav · 2023-04-28T01:18:46Z

src/pages/iou/MoneyRequestDescriptionPage.js

                    title={this.props.translate('common.description')}
-                    shouldShowBackButton
-                    onBackButtonPress={Navigation.goBack}
-                    onCloseButtonPress={this.onCloseButtonPress}


There was IOU.setMoneyRequestDescription(''); call when close page.

0xmiroslav · 2023-04-28T01:19:46Z

src/pages/workspace/WorkspaceInvitePage.js

+                            <HeaderWithBackButton
                                title={this.props.translate('workspace.invite.invitePeople')}
                                subtitle={policyName}
-                                onCloseButtonPress={() => this.clearErrors(true)}


There was this.clearErrors(true) call when close page

0xmiroslav · 2023-04-28T01:22:39Z

src/pages/iou/MoneyRequestModal.js

            title={titleForStep}
-            shouldShowBackButton={shouldShowBackButton}
-            onBackButtonPress={isEditingAmountAfterConfirm ? () => navigateToStep(moneyRequestStepIndex) : navigateToPreviousStep}
+            onBackButtonPress={currentStepIndex === 0 ? Navigation.dismissModal : navigateBack}


There was conditional navigation, so if isEditingAmountAfterConfirm = true, should navigate to request step

This logic is still present, I just decided to move it to navigateBack instead

0xmiroslav · 2023-04-28T01:23:47Z

src/pages/settings/AboutPage/AboutPage.js

+                    <HeaderWithBackButton
                        title={props.translate('initialSettingsPage.about')}
-                        shouldShowBackButton
-                        onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS)}


When navigate to this page directly, it simply closes. Before, it went to Settings page always.

0xmiroslav · 2023-04-28T01:24:10Z

src/pages/settings/Payments/AddPayPalMePage.js

+            <HeaderWithBackButton
                title={props.translate('common.payPalMe')}
-                shouldShowBackButton
-                onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_PAYMENTS)}


0xmiroslav · 2023-04-28T01:27:11Z

src/pages/settings/Profile/TimezoneSelectPage.js

+                        <HeaderWithBackButton
                            title={this.props.translate('timezonePage.timezone')}
-                            shouldShowBackButton
-                            onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_TIMEZONE)}


0xmiroslav · 2023-04-28T01:27:22Z

src/pages/settings/Security/CloseAccountPage.js

+                <HeaderWithBackButton
                    title={this.props.translate('closeAccountPage.closeAccount')}
-                    shouldShowBackButton
-                    onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_SECURITY)}


0xmiroslav · 2023-04-28T01:27:30Z

src/pages/settings/Security/SecuritySettingsPage.js

+            <HeaderWithBackButton
                title={props.translate('initialSettingsPage.security')}
-                shouldShowBackButton
-                onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS)}


0xmiroslav · 2023-04-28T01:27:45Z

src/pages/workspace/WorkspaceInitialPage.js

+                    <HeaderWithBackButton
                        title={props.translate('workspace.common.workspace')}
-                        shouldShowBackButton
-                        onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_WORKSPACES)}


0xmiroslav · 2023-04-28T01:28:11Z

src/pages/workspace/WorkspacesListPage.js

+                <HeaderWithBackButton
                    title={this.props.translate('common.workspaces')}
-                    shouldShowBackButton
-                    onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS)}


mountiny · 2023-05-04T15:06:32Z

@staszekscp lets get this one ready for review and merge it so we can continue with the UP implementation.

staszekscp · 2023-05-05T07:12:39Z

Hey, I've applied requested changes to this PR, because it is needed to continue work with the navigation reboot :)

mountiny · 2023-05-05T15:50:42Z

Closing in favour of #18402

@puneetlath thanks for a review, if you are interested please feel free to review there

staszekscp added 2 commits April 24, 2023 10:57

Fix flatlist glitch for android (#9)

dcd9711

* test flatlist for android * fix linter * fix name * fix not working features * limit rerenders

remove close button

3b5e279

staszekscp requested a review from a team as a code owner April 25, 2023 11:24

melvin-bot bot requested review from puneetlath and removed request for a team April 25, 2023 11:24

Merge branch 'navigation-refactor' into remove-close-button

f200f22

staszekscp changed the title ~~Remove close button~~ Remove close button from HeaderWithCloseButton Apr 25, 2023

puneetlath requested changes Apr 25, 2023

View reviewed changes

mountiny assigned staszekscp Apr 25, 2023

mountiny self-requested a review April 25, 2023 18:20

mountiny added the Ready To Build label Apr 25, 2023

mountiny reviewed Apr 25, 2023

View reviewed changes

0xmiroslav reviewed Apr 25, 2023

View reviewed changes

staszekscp added 2 commits April 26, 2023 08:35

fix linter

19a9513

rename storybook file

a86fdc6

0xmiroslav reviewed Apr 26, 2023

View reviewed changes

mountiny mentioned this pull request Apr 27, 2023

[Navigation Refactor] Fix the Drop to upload feature #18064

Closed

staszekscp added 2 commits April 27, 2023 08:20

Merge branch 'navigation-refactor' into remove-close-button

9aaa242

simplify code

0a5008e

mountiny requested review from 0xmiroslav, mountiny and puneetlath April 27, 2023 06:49

fixes

ab605b0

staszekscp added 2 commits April 27, 2023 10:03

fixes and improvements

ad65cec

remove ModalHeader

9b82116

mountiny approved these changes Apr 27, 2023

View reviewed changes

0xmiroslav reviewed Apr 28, 2023

View reviewed changes

mountiny closed this May 5, 2023

		onThreeDotsButtonPress: () => {},
		shouldShowBackButton: false,

Conversation

staszekscp commented Apr 25, 2023 • edited by mountiny Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

MelvinBot commented Apr 25, 2023

Uh oh!

puneetlath 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!

0xmiroslav Apr 26, 2023 • 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!

puneetlath commented Apr 25, 2023

Uh oh!

mountiny commented Apr 25, 2023

Uh oh!

mountiny 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!

0xmiroslav commented Apr 25, 2023

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!

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!

mountiny commented Apr 26, 2023

Uh oh!

OSBotify commented Apr 26, 2023

Uh oh!

mountiny commented Apr 26, 2023

Uh oh!

0xmiroslav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmiroslav commented Apr 26, 2023

Uh oh!

0xmiroslav commented Apr 26, 2023

Uh oh!

staszekscp commented Apr 25, 2023 •

edited by mountiny

Loading

0xmiroslav Apr 26, 2023 •

edited

Loading

mountiny commented Apr 27, 2023 •

edited

Loading