Fix:: Account details stuck in RHN when switching chats #7725

hiteshagja · 2022-02-14T05:27:18Z

Details

Re-written navigateBackToRootDrawer method to resolve #6720 and related issues if any.

Fixed Issues

Tests

By far I have checked following routes and they worked as expected.
bank-account - working
settings - working
settings/profile - working
settings/preferences - working
settings/security - working
settings/security/closeAccount - working
settings/security/password - working
settings/about - working
settings/about/app-download-links - working
settings/payments - working
settings/payments/add-paypal-me - working
settings/payments/add-debit-card - working
settings/payments/add-bank-account - working
settings/payments/transfer-balance - working
settings/payments/choose-transfer-account - working
new/group - working
new/chat - working
r/:reportID - working
iou/request - working
iou/request/:reportID - working
iou/split - working
iou/split/:reportID - working
iou/send - working
iou/split/currency - working
iou/split/currency/:reportID - working
iou/send/currency - working
iou/send/add-bank-account - working
iou/send/add-debit-card - working
iou/send/enable-payments - working
iou/details/add-bank-account - working
iou/details/add-debit-card - working
iou/details/enable-payments - working
iou/details/${chatReportID}/${iouReportID} - working
search - working
details?login=${encodeURIComponent(login)} - working
r/:reportID/participants - working
r/${reportID}/participants/details?login=${encodeURIComponent(login)} - working
r/:reportID/details - working
r/:reportID/settings - working
iou/send/currency/:reportID - working

QA Steps

Here are many of the routes which I checked with following steps

Home -> go to any report -> click on header -> open detail -> ctrl+K -> search -> select any report
Home -> ctrl+k -> search -> select any report
Home -> click on search icon -> search -> select any report
Home -> click on groupReport -> click on header -> open group detail -> click on any member of group profile -> detail open -> ctl+k -> search -> select any report
Home -> click on profile -> setting -> ctrl + k -> search -> select any report
Home -> click on plus icon -> start new chart -> ctrl + k -> search -> select any report
Home -> click on plus icon -> split bill -> enter amount -> select user -> split
Home -> setting -> change password -> update password
Home -> setting -> Profile -> update Profile
Home -> setting -> workspace -> general setting -> click on header (?) -> get assistance -> chat with concierge
Home -> setting -> workspace -> click on more(3 dot) on header -> delete workspace

Tested On

Web
Android
iOS
Desktop

Screenshots

Web

Untitled.2.mp4

Android

Untitled.mp4

iOS

ios-expensify_xdLqyc9d.mov

Desktop

expensifyDesktopApp.mp4

github-actions · 2022-02-14T05:27:36Z

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

hiteshagja · 2022-02-14T05:44:37Z

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

parasharrajat

Please add Screenshots/clips for all platforms. It is a must. Add QA and testing steps to test this change.
As this change affects the whole app. Please be detailed in your QA steps and cover most of the different navigation cases. LHN, RHN, two Side-modals as per the issue, etc.

Don't forget to sign your commits.

parasharrajat · 2022-02-14T07:47:38Z

src/libs/Navigation/CustomActions.js

+    const activeState = getActiveState();
+    navigationRef.current.dispatch({
+        ...StackActions.popToTop(),
+        target: activeState.key,


Please add a descriptive comment about what is happening here. And also mention why does this works and when will it not work.

Code comment added.

parasharrajat

Please sign your commits and add videos for all platforms and tick those checkboxes in the template.

parasharrajat · 2022-02-14T15:56:25Z

src/libs/Navigation/CustomActions.js

+/**
+ * @returns {Object}
+ */
+function getActiveState() {


There is a duplicate method for this....Please remove the other one.

Thanks rajat. Just wanted to add to remember to always test your PR before putting it out for review. If you test the current code on ios you'll see,

which would quickly guide you to remove the extra method. Additionally, if you need more time you can always create a draft PR first without any assignees such that once its ready for review you can click on the ready for review button and it will then auto assign the reviewers.

parasharrajat · 2022-02-14T16:02:29Z

src/libs/Navigation/CustomActions.js

+    //Dispatch "popToTop" action takes you back to the first screen in the stack at the same time 
+    //it will prevent bubbling of action and limit it to activeState by specifing target.


Suggested change

//Dispatch "popToTop" action takes you back to the first screen in the stack at the same time

//it will prevent bubbling of action and limit it to activeState by specifing target.

// To navigate to the main drawer route, pop to the first route on the Root Stack Navigator as the main drawer is always the first route that is activated.

// It will pop all fullscreen and RHN modals that are over the main drawer.

// It won't work when the main drawer is not the first route of the Root Stack Navigator which is not the case ATM.

chiragsalian · 2022-02-14T22:45:40Z

Also @parasharrajat, locally i removed the extra getActiveState and tested the flow mentioned here.

Is this the correct expectation?
https://user-images.githubusercontent.com/14356145/153959753-7726a35e-bb8b-46e8-9910-e5b3626a5dd5.mov

parasharrajat · 2022-02-15T11:00:12Z

@hiteshagja You would have to sign all of your commits before we can merge this. Also, a test on each platform is required. Please add the remaining platforms back to the description and tick them when tested.

parasharrajat · 2022-02-15T11:05:48Z

@chiragsalian It does not seem like Be taken to directly NewDot setup bank account page is passing from your steps. But this Pr only affects going back to the main drawer which can be seen working in your video. So I think it is a pass.

As a side note, we should look at why we are not landing on the bank page(if It should). I am actually sure what is that feature. Maybe Rory can help here. But this is surely out of scope of this PR.

hiteshagja · 2022-02-15T13:12:28Z

@hiteshagja You would have to sign all of your commits before we can merge this. Also, a test on each platform is required. Please add the remaining platforms back to the description and tick them when tested.

@parasharrajat I am testing for iOS for now and will update you for the same. Also I tried to sign old commits but command didn't work in 6720 branch with some strange reason. However, it's working fine if I create new branch.

So, shall I close this repo and create new branch with single commit in it and raise new PR?

parasharrajat · 2022-02-15T13:16:39Z

There is a small command to sign existing commits https://superuser.com/a/1123928/1035336. I don't think it is a good idea to close this PR and create new ones. We have discussed some important stuff here.

During the rebase please merge the duplicate commits as well. I see there are some on this PR.

let me know if you have some trouble with it.

… Also removed duplicate getActiveState method.

hiteshagja · 2022-02-15T13:54:31Z

There is a small command to sign existing commits https://superuser.com/a/1123928/1035336. I don't think it is a good idea to close this PR and create new ones. We have discussed some important stuff here.

During the rebase please merge the duplicate commits as well. I see there are some on this PR.

let me know if you have some trouble with it.

Thanks!

signed all commits.

parasharrajat

LGTM.

cc: @chiragsalian.

Leaving final testing to Chirag.

🎀 👀 🎀 C+ reviewed

hiteshagja · 2022-02-16T11:47:06Z

Shall I commit for that lint error?

parasharrajat · 2022-02-16T13:21:40Z

Yes. Thanks.

chiragsalian

LGTM

OSBotify · 2022-02-17T20:10:10Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

hiteshagja · 2022-02-18T06:25:46Z

@parasharrajat , just wanted to check, in which upwork job I have to submit my proposal?

OSBotify · 2022-02-23T21:59:48Z

🚀 Deployed to staging by @chiragsalian in version: 1.1.40-0 🚀

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

OSBotify · 2022-03-01T18:50:07Z

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

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

Stutikuls · 2022-03-08T12:59:15Z

Issue 1 -

Title-[Medium] Chrome + Jaws: Screen reader : Focus does not lands on the new expanded content after activating the setting control.
Actual - Focus does not lands on the new expanded content of the setting control, focus moves to the next element (List elements) after activating the setting control.
Expected - After activating the setting control focus lands on the close(X) button of the new expanded window.
WCAG failure - 2.4.3
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Web(Chrome + Jaws), Desktop(MAC)

7725_Focus.does.not.lands.on.the.expanded.conent.mp4

hiteshagja requested a review from a team as a code owner February 14, 2022 05:27

MelvinBot requested review from chiragsalian and parasharrajat and removed request for a team February 14, 2022 05:27

parasharrajat suggested changes Feb 14, 2022

View reviewed changes

hiteshagja force-pushed the 6720 branch 3 times, most recently from 1c9ec59 to 7749bb4 Compare February 15, 2022 12:37

hiteshagja added 3 commits February 15, 2022 19:16

Improvement in navigateBackToRootDrawer method in CustomActions

d42410d

Code comment and explaination for navigationRef.current.dispatch.

14a938b

Code comment updated for navigationRef.current.dispatch explaination.…

4db894e

… Also removed duplicate getActiveState method.

hiteshagja force-pushed the 6720 branch from 7749bb4 to 4db894e Compare February 15, 2022 13:48

parasharrajat previously approved these changes Feb 15, 2022

View reviewed changes

trailing space lint error resolved.

aa5f9a4

hiteshagja dismissed parasharrajat’s stale review via aa5f9a4 February 16, 2022 13:35

parasharrajat approved these changes Feb 16, 2022

View reviewed changes

chiragsalian approved these changes Feb 17, 2022

View reviewed changes

chiragsalian merged commit 4b04f73 into Expensify:main Feb 17, 2022

OSBotify mentioned this pull request Feb 23, 2022

Deploy Checklist: New Expensify 2022-02-23 #7883

Closed

69 tasks

botify mentioned this pull request Mar 1, 2022

[HOLD for payment 2022-03-08] [$500] Account details stuck in RHN when switching chats #6720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix:: Account details stuck in RHN when switching chats #7725

Fix:: Account details stuck in RHN when switching chats #7725

hiteshagja commented Feb 14, 2022 •

edited

Loading

github-actions bot commented Feb 14, 2022 •

edited

Loading

hiteshagja commented Feb 14, 2022

parasharrajat left a comment •

edited

Loading

parasharrajat Feb 14, 2022

hiteshagja Feb 14, 2022

parasharrajat left a comment •

edited

Loading

parasharrajat Feb 14, 2022

chiragsalian Feb 14, 2022

parasharrajat Feb 14, 2022 •

edited

Loading

chiragsalian commented Feb 14, 2022 •

edited

Loading

parasharrajat commented Feb 15, 2022 •

edited

Loading

parasharrajat commented Feb 15, 2022

hiteshagja commented Feb 15, 2022

parasharrajat commented Feb 15, 2022 •

edited

Loading

hiteshagja commented Feb 15, 2022

parasharrajat left a comment

hiteshagja commented Feb 16, 2022

parasharrajat commented Feb 16, 2022

chiragsalian left a comment

OSBotify commented Feb 17, 2022

hiteshagja commented Feb 18, 2022

OSBotify commented Feb 23, 2022

OSBotify commented Mar 1, 2022

Stutikuls commented Mar 8, 2022 •

edited

Loading

		//Dispatch "popToTop" action takes you back to the first screen in the stack at the same time
		//it will prevent bubbling of action and limit it to activeState by specifing target.

-    //Dispatch "popToTop" action takes you back to the first screen in the stack at the same time
-    //it will prevent bubbling of action and limit it to activeState by specifing target.
+    // To navigate to the main drawer route, pop to the first route on the Root Stack Navigator as the main drawer is always the first route that is activated.
+    // It will pop all fullscreen and RHN modals that are over the main drawer.
+   // It won't work when the main drawer is not the first route of the Root Stack Navigator which is not the case ATM.

Fix:: Account details stuck in RHN when switching chats #7725

Fix:: Account details stuck in RHN when switching chats #7725

Conversation

hiteshagja commented Feb 14, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Android

iOS

Desktop

github-actions bot commented Feb 14, 2022 • edited Loading

hiteshagja commented Feb 14, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat Feb 14, 2022

Choose a reason for hiding this comment

hiteshagja Feb 14, 2022

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat Feb 14, 2022

Choose a reason for hiding this comment

chiragsalian Feb 14, 2022

Choose a reason for hiding this comment

parasharrajat Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

chiragsalian commented Feb 14, 2022 • edited Loading

parasharrajat commented Feb 15, 2022 • edited Loading

parasharrajat commented Feb 15, 2022

hiteshagja commented Feb 15, 2022

parasharrajat commented Feb 15, 2022 • edited Loading

hiteshagja commented Feb 15, 2022

parasharrajat left a comment

Choose a reason for hiding this comment

hiteshagja commented Feb 16, 2022

parasharrajat commented Feb 16, 2022

chiragsalian left a comment

Choose a reason for hiding this comment

OSBotify commented Feb 17, 2022

hiteshagja commented Feb 18, 2022

OSBotify commented Feb 23, 2022

OSBotify commented Mar 1, 2022

Stutikuls commented Mar 8, 2022 • edited Loading

hiteshagja commented Feb 14, 2022 •

edited

Loading

github-actions bot commented Feb 14, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Feb 14, 2022 •

edited

Loading

chiragsalian commented Feb 14, 2022 •

edited

Loading

parasharrajat commented Feb 15, 2022 •

edited

Loading

parasharrajat commented Feb 15, 2022 •

edited

Loading

Stutikuls commented Mar 8, 2022 •

edited

Loading