Fix: Missing PayPal account when trying to delete it #7571

ahmdshrif · 2022-02-04T19:54:28Z

Details

Fixed Issues

$ #7413

Tests

Verify that no errors appear in the JS console

QA Steps

Launch the app and login
Open your settings
Open the "Payments" page
Tap my PayPal payment
Tap Delete

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS (iPad)

Android

parasharrajat · 2022-02-04T20:42:54Z

src/pages/settings/Payments/PaymentsPage.js

@@ -290,7 +290,7 @@ class PaymentsPage extends React.Component {
                                !this.props.isSmallScreenWidth ? styles.sidebarPopover : '',
                            ]}
                        >
-                            {this.props.isSmallScreenWidth && (
+                            {(Platform.OS !== 'web' || this.props.isSmallScreenWidth) && (


Please read the style guide. We don't do this. If you need to run something on native then created a index.native.js file.

Have you tested this on Ipad or Android Tab?

@parasharrajat yes test it and work fine on iPad.
I think of doing this change.

render() { ... const isPopoverBottomMounte = this.props.anchorPositionTop === 0 || this.props.isSmallScreenWidth

Suggested change

{(Platform.OS !== 'web' || this.props.isSmallScreenWidth) && (

{isPopoverBottomMounte && (

sense model comes from down in case of this value with 0 or issmalScreen true.

this value is 0 in native all time.

this is the new change I mention from iPad

Do you think we can use props.isMediumScreenWidth here?

not work with landscape on the tablet.

we need a Paypal account to appear when the Popover comes from the bottom

and in native it comes from a bottom in all cases.
on the web comes from a bottom when the screen is small only.

so that is why I go with this change

Ok. But depending on this condition is a hack. We should not depend on unrelated props. it can break easily.

If you want to show this on the native and Mobile web, then create a utility like canFocusInputOnScreenFocus.

a Name could be ShouldShowOnBottomDockedPopover

it's not an unrelated state if you change the this.state.anchorPositionTop to not be 0 then
the popover will not be from the bottom and then we will need to disable the Paypal account view.
like on the web here.

so I don't prefer to make a new utility for it may be in the future we need to not render it from the bottom in some native cases.
we will just change the state anchorPositionTop this will render the popover from click place and will disable Paypal view as expected. on one change.

Ok. I don't mind this as well. Leaving this to @thienlnam.

I don't mind this solution, but can we get a better comment to clarify what this condition means? Variable name alone is not clear enough for me

@thienlnam yes , i add it now

parasharrajat

LGTM.

cc: @thienlnam
🎀 👀 🎀 C+ reviewed

thienlnam · 2022-02-08T18:43:52Z

src/pages/settings/Payments/PaymentsPage.js

@@ -216,6 +216,9 @@ class PaymentsPage extends React.Component {

    render() {
        const isPayPalMeSelected = this.state.formattedSelectedPaymentMethod.type === CONST.PAYMENT_METHODS.PAYPAL;
+
+        // determine if the popover appears from the bottom of the screen or not.


Looking for a more detailed comment, something along the lines of
Determines whether or not the modal popup is mounted from the bottom of the screen instead of the side mount on Web or Desktop screens

looks better, i update it now

thienlnam

Awesome, thanks for adding that

OSBotify · 2022-02-09T17:33:44Z

✋ 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-02-15T19:33:22Z

🚀 Deployed to staging by @thienlnam in version: 1.1.39-0 🚀

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

OSBotify · 2022-02-23T21:22:41Z

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

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

Stutikuls · 2022-03-04T12:59:01Z

Issue 1 -

Title- [Medium]: Chrome +Jaws : Screen reader : Role is not defined for Paypal A/c.
Actual - When focus lands on the Paypal A/c, screen reader is reading "Paypal.me testetstes(A/c name)", not read the role for the A/c.
Expected - Role should be define as a 'button' for the paypal a/c and screen reader should read like "Paypal.me testetstes".
WCAG failure - 4.1.2
Reproducible in staging?: - Yes
Version Number: - v1.1.39-3
Platforms - Web, Desktop, Mobile-web, iOS

7571_Role.is.not.define.for.Paypal.Account.mp4

Issue 2-

Title- [Medium]: Chrome + Jaws: Keyboard Navigation :Order is not sequential on 'Delete dialogue'
Actual - After open the dialogue first focus lands on the Cancel control then it's move to delete control.
Expected - Focus should first land on the 'Delete' control then should lands on the 'Cancel'.
WCAG failure - 2.4.3
Reproducible in staging?: - Yes
Version Number: - v1.1.39-3
Platforms - Web, Desktop

7571_Oreder.is.not.sequential.mp4

Issue 3 -

Title- [Medium]: Chrome +Jaws : Screen reader : Role is not defined for 'Delete' control.
Actual - When focus lands on the Delete control, screen reader is reading "Delete", not read the role.
Expected - Role should be define as 'button' and screen reader should read like "Delete button".
WCAG failure - 4.1.2
Reproducible in staging?: - Yes
Version Number: - v1.1.39-3
Platforms - Web, Mobile-web, iOS

7571_Role.is.not.defined.for.Delete.control.mp4

Issue 4 -

Title- [Medium]: Chrome +Jaws : Screen reader : Screen reader is not reading the Dialogue information.
Actual - After select the delete control, screen reader is reading "Cancel", not read the 'Are you sure...".
Expected - Screen reader should read like "Are you sure that you want to delete this account?, cancel button".
WCAG failure - 1.3.1
Reproducible in staging?: - Yes
Version Number: - v1.1.39-3
Platforms - Web

7571_Screen.reader.is.not.reading.the.dialouge.information.along.with.the.controls.mp4

ogumen · 2022-03-07T10:16:24Z

Hey @Stutikuls, please report these bugs in the #expensify-open-source channel! https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#finding-jobs

Hi @thienlnam, the accessibility team is working on the in-sprint testing of the PRs from the accessibility perspective. Initially we were told to only leave comments with all issues we have found on the corresponding PRs. Has the process changed lately?
CC @roryabraham @stitesExpensify @Stutikuls

roryabraham · 2022-03-07T17:46:40Z

Initially we were told to only leave comments with all issues we have found on the corresponding PRs. Has the process changed lately?

For now, please keep just posting bugs in the issue as you have been. The process will change once we have made substantial efforts to provide better accessibility in our app and have established some code patterns to do so.

check web platform

50bd821

ahmdshrif changed the title ~~check web platform~~ fix: Missing PayPal account when trying to delete it Feb 4, 2022

ahmdshrif marked this pull request as ready for review February 4, 2022 20:10

ahmdshrif requested a review from a team as a code owner February 4, 2022 20:10

botify requested review from parasharrajat and thienlnam February 4, 2022 20:10

MelvinBot removed the request for review from a team February 4, 2022 20:10

parasharrajat suggested changes Feb 4, 2022

View reviewed changes

ahmdshrif added 2 commits February 6, 2022 16:32

check popover mount postion

0c54037

fix isPopoverBottomMount

624aabb

ahmdshrif requested a review from parasharrajat February 6, 2022 14:46

parasharrajat previously approved these changes Feb 7, 2022

View reviewed changes

add comment to isPopoverBottomMount

e73f0b2

ahmdshrif dismissed parasharrajat’s stale review via e73f0b2 February 8, 2022 05:49

ahmdshrif requested a review from parasharrajat February 8, 2022 05:50

thienlnam requested changes Feb 8, 2022

View reviewed changes

update comment

4673bf5

ahmdshrif requested a review from thienlnam February 8, 2022 18:56

thienlnam approved these changes Feb 8, 2022

View reviewed changes

parasharrajat approved these changes Feb 9, 2022

View reviewed changes

thienlnam merged commit 1e181db into Expensify:main Feb 9, 2022

NicMendonca mentioned this pull request Feb 15, 2022

[HOLD for payment 2022-03-02] IPAD - Payment - Missing PayPal account when trying to delete it #7413

Closed

OSBotify mentioned this pull request Feb 15, 2022

Deploy Checklist: New Expensify 2022-02-15 #7759

Closed

85 tasks

michaelhaxhiu changed the title ~~fix: Missing PayPal account when trying to delete it~~ Fix: Missing PayPal account when trying to delete it Feb 23, 2022

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Missing PayPal account when trying to delete it #7571

Fix: Missing PayPal account when trying to delete it #7571

ahmdshrif commented Feb 4, 2022 •

edited

Loading

parasharrajat Feb 4, 2022

ahmdshrif Feb 4, 2022 •

edited

Loading

ahmdshrif Feb 4, 2022

parasharrajat Feb 7, 2022

ahmdshrif Feb 7, 2022 •

edited

Loading

parasharrajat Feb 7, 2022

ahmdshrif Feb 7, 2022 •

edited

Loading

parasharrajat Feb 7, 2022

thienlnam Feb 7, 2022

ahmdshrif Feb 8, 2022

parasharrajat left a comment

thienlnam Feb 8, 2022

ahmdshrif Feb 8, 2022

thienlnam left a comment

OSBotify commented Feb 9, 2022

OSBotify commented Feb 15, 2022

OSBotify commented Feb 23, 2022

Stutikuls commented Mar 4, 2022 •

edited

Loading

This comment was marked as resolved.

ogumen commented Mar 7, 2022

roryabraham commented Mar 7, 2022

	{(Platform.OS !== 'web' \|\| this.props.isSmallScreenWidth) && (
	{isPopoverBottomMounte && (

Fix: Missing PayPal account when trying to delete it #7571

Fix: Missing PayPal account when trying to delete it #7571

Conversation

ahmdshrif commented Feb 4, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS (iPad)

Android

Choose a reason for hiding this comment

ahmdshrif Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmdshrif Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmdshrif Feb 7, 2022 • edited Loading

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

parasharrajat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thienlnam left a comment

Choose a reason for hiding this comment

OSBotify commented Feb 9, 2022

OSBotify commented Feb 15, 2022

OSBotify commented Feb 23, 2022

Stutikuls commented Mar 4, 2022 • edited Loading

This comment was marked as resolved.

ogumen commented Mar 7, 2022

roryabraham commented Mar 7, 2022

ahmdshrif commented Feb 4, 2022 •

edited

Loading

ahmdshrif Feb 4, 2022 •

edited

Loading

ahmdshrif Feb 7, 2022 •

edited

Loading

ahmdshrif Feb 7, 2022 •

edited

Loading

Stutikuls commented Mar 4, 2022 •

edited

Loading