Update the IOU "Cancel" button #8877

thesahindia · 2022-05-04T21:42:29Z

Details

Changed the way we show ActivityIndicator inside button. If isLoading is true the text for button will be rendered behind the ActivityIndicator so that button width doesn't change when it's in loading state.
Size of iou cancel/decline button changes while loading, this PR fixes that.

Fixed Issues

$ #8715

Tests | QA Steps

Go to a chat and request money
Check the iou details and cancel the request
Verify the button width doesn't change
Follow the same steps above but with language set to spanish

Go to a chat and request money from account b
Now login to account b
Check the iou request
Verify the button width doesn't change
Follow the same steps above but with language set to spanish
Sign out
Enter email or phone that doesn't belong to an existing account
Press "Continue"
Tap on resend link button
Verify the button width doesn't change
Do the same again but with language set to spanish

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Screenshots

Web

Screen.Recording.2022-05-18.at.11.35.22.PM.mov

Mobile Web

Screen.Recording.2022-05-18.at.11.37.53.PM.mov

Desktop

Screen.Recording.2022-05-18.at.11.56.23.PM.mov

iOS

Screen.Recording.2022-05-18.at.11.36.12.PM.mov

Android

Screen.Recording.2022-05-18.at.11.45.32.PM.mov

roryabraham

LGTM, but I'll let @Santhosh-Sellavel test and review 👍

Santhosh-Sellavel · 2022-05-10T02:33:35Z

Just waiting for this thread here to conclude.

Santhosh-Sellavel

We are looking for another approach as discussed! https://expensify.slack.com/archives/C01GTK53T8Q/p1652390743738389?thread_ts=1651695607.990559&cid=C01GTK53T8Q

…u-button merge main

src/components/Button.js

thesahindia · 2022-05-18T18:55:01Z

src/pages/signin/ResendValidationForm.js

@@ -143,7 +143,6 @@ class ResendValidationForm extends React.Component {
                        text={this.props.translate('resendValidationForm.resendLink')}
                        isLoading={this.props.account.loading}
                        onPress={this.validateAndSubmitForm}
-                        style={styles.resendLinkButton}


removed the minWidth given here as we don't need this one now.

thesahindia · 2022-05-18T19:11:47Z

Just after pressing the "resend link" button we are showing "Link has been re-sent" but it's kinda weird that the resend link button keeps loading for a while.

What you think @roryabraham

Screen.Recording.2022-05-19.at.12.27.59.AM.mov

src/components/Button.js

thesahindia · 2022-05-27T06:15:32Z

bump @Santhosh-Sellavel, the review is still pending.

Santhosh-Sellavel · 2022-05-27T15:52:33Z

I will get this soon, @thesahindia can you resolve the conflicts!

src/styles/styles.js

thesahindia · 2022-05-30T18:17:11Z

Done!

Santhosh-Sellavel · 2022-05-31T19:58:18Z

@thesahindia
Loading Indicator is not prominent as before, looks dull!

Before

Screen.Recording.2022-06-01.at.1.25.56.AM.mov

Now

Screen.Recording.2022-06-01.at.1.23.50.AM.mov

cc: @roryabraham

roryabraham · 2022-05-31T20:10:54Z

src/components/Button.js

@@ -273,6 +270,12 @@ class Button extends Component {
                        ]}
                    >
                        {this.renderContent()}
+                        {this.props.isLoading && (
+                        <ActivityIndicator


This should be indented 4 more spaces.

roryabraham · 2022-05-31T20:11:41Z

Sorry, meant to hit Request Changes, not Approve

roryabraham

Approved by mistake

roryabraham · 2022-05-31T20:13:59Z

Loading Indicator is not prominent as before, looks dull!

Agree, we should switch the color of the ActivityIndicator from themeColors.textReversed to themeColors.text

thesahindia · 2022-05-31T20:19:28Z

Agree, we should switch the color of the ActivityIndicator from themeColors.textReversed to themeColors.text

We can add a new prop for passing a different color and and we can keep themeColors.textReversed as default because themeColors.text won't look good on green buttons.

Santhosh-Sellavel · 2022-05-31T20:20:28Z

Loading Indicator is not prominent as before, looks dull!

Agree, we should switch the color of the ActivityIndicator from themeColors.textReversed to themeColors.text

but it would affect other buttons
Ex: Save button

@roryabraham

Santhosh-Sellavel · 2022-05-31T20:23:52Z

Agree, we should switch the color of the ActivityIndicator from themeColors.textReversed to themeColors.text

We can add a new prop for passing a different color and and we can keep themeColors.textReversed as default because themeColors.text won't look good on green buttons.

This bit makes feel more over engineered solution but there isn't any other way.

Okay for me 👍

Cc: @roryabraham

roryabraham · 2022-06-01T00:28:49Z

Ah, good point @thesahindia. Rather than adding another prop, can we just do something like:

<ActivityIndicator color={(this.props.success || this.props.danger) ? themeColors.textReversed : themeColors.text} />

?

thesahindia · 2022-06-01T06:35:49Z

can we just do something like:

<ActivityIndicator color={(this.props.success || this.props.danger) ? themeColors.textReversed : themeColors.text} />

Oh yeah I didn't think about this, this is better 👍🏼

Santhosh-Sellavel · 2022-06-01T14:47:51Z

#### PR Reviewer Checklist

I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 verified proper code patterns were followed (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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 verified that this PR follows the guidelines as stated in the Review Guidelines
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
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 */
- Any functional components have the displayName property
- 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 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 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.

Santhosh-Sellavel

Looks good, and tests well for me!

Santhosh-Sellavel · 2022-06-01T14:51:09Z

@roryabraham all you!

roryabraham

Thanks, LGTM 👍🏼

OSBotify · 2022-06-01T15:26:38Z

✋ 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 · 2022-06-01T17:34:34Z

🚀 Deployed to staging by @roryabraham in version: 1.1.70-2 🚀

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

OSBotify · 2022-06-01T18:15:47Z

🚀 Deployed to staging by @roryabraham in version: 1.1.70-2 🚀

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

OSBotify · 2022-06-01T18:53:47Z

🚀 Deployed to staging by @roryabraham in version: 1.1.70-2 🚀

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

OSBotify · 2022-06-06T17:38:32Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.71-2 🚀

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

add minimum width

4fb557f

thesahindia requested a review from a team as a code owner May 4, 2022 21:42

melvin-bot bot requested review from roryabraham and Santhosh-Sellavel and removed request for a team May 4, 2022 21:42

roryabraham previously approved these changes May 6, 2022

View reviewed changes

Santhosh-Sellavel suggested changes May 17, 2022

View reviewed changes

thesahindia added 4 commits May 19, 2022 00:05

Merge branch 'main' of github.com:thesahindia/App into thesahindia/io…

470d18b

…u-button merge main

show the activity indicator above the hidden text

392287d

use button

f49aaa2

remove minimum width

f457cb6

thesahindia dismissed roryabraham’s stale review via f457cb6 May 18, 2022 18:52

thesahindia commented May 18, 2022

View reviewed changes

src/components/Button.js Show resolved Hide resolved

thesahindia commented May 18, 2022

View reviewed changes

Santhosh-Sellavel suggested changes May 23, 2022

View reviewed changes

src/components/Button.js Show resolved Hide resolved

Santhosh-Sellavel reviewed May 30, 2022

View reviewed changes

src/styles/styles.js Outdated Show resolved Hide resolved

resolve conflicts

2bedb84

roryabraham approved these changes May 31, 2022

View reviewed changes

roryabraham requested changes May 31, 2022

View reviewed changes

use differnt color for gray button

3e0ba3a

Santhosh-Sellavel approved these changes Jun 1, 2022

View reviewed changes

roryabraham approved these changes Jun 1, 2022

View reviewed changes

roryabraham merged commit e05524c into Expensify:main Jun 1, 2022

OSBotify mentioned this pull request Jun 1, 2022

Deploy Checklist: New Expensify 2022-06-01 #9275

Closed

27 tasks

This was referenced Jun 2, 2022

[med] Chrome + Jaws: Role is not defined for Payment method controls. #8618

Closed

[med] Chrome + Jaws: Success message after uploading image is not announced. #8624

Closed

melvin-bot bot mentioned this pull request Jun 6, 2022

[HOLD for payment 2022-06-13] [$250] Update the IOU "Cancel" button so that it has a fixed size - reported by @Puneet-here #8715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the IOU "Cancel" button #8877

Update the IOU "Cancel" button #8877

thesahindia commented May 4, 2022 •

edited

Loading

roryabraham left a comment

Santhosh-Sellavel commented May 10, 2022

Santhosh-Sellavel left a comment

thesahindia May 18, 2022

thesahindia commented May 18, 2022

thesahindia commented May 27, 2022

Santhosh-Sellavel commented May 27, 2022

thesahindia commented May 30, 2022

Santhosh-Sellavel commented May 31, 2022

roryabraham May 31, 2022

roryabraham commented May 31, 2022

roryabraham left a comment

roryabraham commented May 31, 2022

thesahindia commented May 31, 2022

Santhosh-Sellavel commented May 31, 2022 •

edited

Loading

Santhosh-Sellavel commented May 31, 2022

roryabraham commented Jun 1, 2022

thesahindia commented Jun 1, 2022 •

edited

Loading

Santhosh-Sellavel commented Jun 1, 2022 •

edited

Loading

Santhosh-Sellavel left a comment

Santhosh-Sellavel commented Jun 1, 2022

roryabraham left a comment

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 6, 2022

Update the IOU "Cancel" button #8877

Update the IOU "Cancel" button #8877

Conversation

thesahindia commented May 4, 2022 • edited Loading

Details

Fixed Issues

Tests | QA Steps

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web

Desktop

iOS

Android

roryabraham left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented May 10, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

thesahindia May 18, 2022

Choose a reason for hiding this comment

thesahindia commented May 18, 2022

thesahindia commented May 27, 2022

Santhosh-Sellavel commented May 27, 2022

thesahindia commented May 30, 2022

Santhosh-Sellavel commented May 31, 2022

Before

Now

roryabraham May 31, 2022

Choose a reason for hiding this comment

roryabraham commented May 31, 2022

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham commented May 31, 2022

thesahindia commented May 31, 2022

Santhosh-Sellavel commented May 31, 2022 • edited Loading

Santhosh-Sellavel commented May 31, 2022

roryabraham commented Jun 1, 2022

thesahindia commented Jun 1, 2022 • edited Loading

Santhosh-Sellavel commented Jun 1, 2022 • edited Loading

#### PR Reviewer Checklist

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Jun 1, 2022

roryabraham left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 6, 2022

thesahindia commented May 4, 2022 •

edited

Loading

Santhosh-Sellavel commented May 31, 2022 •

edited

Loading

thesahindia commented Jun 1, 2022 •

edited

Loading

Santhosh-Sellavel commented Jun 1, 2022 •

edited

Loading