fix: navigation for exisiting report screen #5131

parasharrajat · 2021-09-08T08:18:09Z

Details

Fixed Issues

$ #4545

Tests | QA Steps

Open New Dot on Mobile.
Go to Settings -> About.
Click on Report a bug.
Concierge chat will be opened now.
Now repeat steps 1 & 2.

Tested On

Screenshots

Web | Desktop

report-wd.mp4

Mobile Web

report-m.mp4

iOS

Android

report-a.mp4

marcaaron · 2021-09-08T15:56:52Z

src/libs/Navigation/CustomActions.js

@@ -24,7 +24,19 @@ function pushDrawerRoute(screenName, params, navigationRef) {

        if (activeReportID === params.reportID) {
            if (state.type !== 'drawer') {
-                navigationRef.current.dispatch(StackActions.pop());
+                navigationRef.current.dispatch(() => {
+                    // If there are multiple routes then we can pop back to the first route


This code looks familiar... 😄

Are you OK with just copying and pasting things from other places?
Or should we think of a better way to handle this duplication?

Sorry about that. I just found this to be straight to start with this. I can definitely refactor it into a function. 😄

I am not sure the best way to dry this up. I can basically call the DismissModal here instead. It does basically the same thing. What do you suggest?

Please take some time to look into it and propose some options?

Sure. Thanks.

parasharrajat

Code Updated. Please review.

parasharrajat · 2021-09-14T22:10:59Z

src/libs/Navigation/Navigation.js

-    // Navigate back to where we were before we launched the modal
-    goBack(shouldOpenDrawer);
-
-    if (!normalizedShouldOpenDrawer) {
-        return;
-    }
-
-    openDrawer();


@marcaaron I didn't understand this part. Why were this code opening Drawer two times?

via goback when shouldOpenDrawer is true.

Then OpenDrawer call which happens when shouldOpenDrawer is true.

Can you rephrase this question? I'm not really sure what you are asking.

I looked at the code. It seems to me that this piece of code was calling openDrawer two times.

Via goback when shouldOpenDrawer is true.

Then OpenDrawer call which happens when shouldOpenDrawer is true.

Or am I wrong?

marcaaron · 2021-09-14T22:20:01Z

Just a heads up this is very far down my list of priorities and will take some time to review.
Most likely I will be able to get to it within 2 weeks.

kadiealexander · 2021-09-16T04:46:56Z

Hi @parasharrajat, I've placed this issue on hold as per this update, as we are prioritising issues related to a feature release scheduled for 9/24. As an apology for the delay, we will add a $100 bonus as a thank you for waiting.

parasharrajat · 2021-10-18T19:48:35Z

@marcaaron Ready for review.

marcaaron

Really like the clean up here and eliminating the need to explicitly call Navigation.dismissModal() in some places. But I have some thoughts and suggestions about how to make the code clearer.

marcaaron · 2021-10-18T20:19:29Z

src/libs/Navigation/CustomActions.js

+ * Go back to the Main Drawer
+ * @param {Object} navigationRef
+ */
+function navigateBackToDrawer(navigationRef) {


I'm having trouble with this name because it implies we are calling this when the drawer is not active. It looks like something I would call if I was not on the drawer to take me back to the drawer. However, if you look we also have logic for handling when a nested drawer is active.

Ultimately, I find handling both responsibilities confusing and would love to figure out an alternative.

I didn't find any good name for this method. To me, it seems to do multiple things. But I just removed the duplicate behaviour from our navigation and put it into one method. I found it difficult to break this into two and still abstract away the duplicates.

I will try to find something cleaner.

Ok no worries. I think with the better naming this is acceptable now.

marcaaron · 2021-10-18T20:21:54Z

src/libs/Navigation/CustomActions.js

@@ -24,7 +60,7 @@ function pushDrawerRoute(screenName, params, navigationRef) {

        if (activeReportID === params.reportID) {
            if (state.type !== 'drawer') {
-                navigationRef.current.dispatch(StackActions.pop());
+                navigateBackToDrawer(navigationRef);


One thing that sticks out to me is that we are checking state.type !== 'drawer' and then again checking in navigateBackToDrawer(). So it seems the method has more responsibility than it needs.

Yeah. I think its more cleaner if we change the name to navigateBackToRootDrawer or navigateBackToReportDrawer.

marcaaron · 2021-10-18T20:26:31Z

src/libs/Navigation/CustomActions.js

+        if (state.type === 'drawer') {
+            isLeavingDrawerNavigator = true;
+            return StackActions.pop();
+        }


Do we still have nested drawer navigators in use? I think we got rid of the one case. In any case, I think if we want to keep this logic we should make a distinction between a "nested drawer navigator" and a "main" or "root" drawer navigator.

Otherwise, these variable names are confusing. I understand this code but someone might have a thought like, "Why do we have isLeavingDrawerNavigator in a method called navigateBackToDrawer()? Shouldn't isLeavingDrawerNavigator always be false?" Maybe it should be isLeavingNestedDrawerNavigator instead.

Yeah, I agree.

marcaaron

Thanks for the updates 👍

OSBotify · 2021-10-18T21:47:31Z

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

parasharrajat · 2021-10-18T22:03:58Z

src/libs/Navigation/CustomActions.js

@@ -52,4 +88,5 @@ function pushDrawerRoute(screenName, params, navigationRef) {

 export default {
    pushDrawerRoute,
+    navigateBackToDrawer: navigateBackToRootDrawer,


@marcaaron Oh, my editor messed it up. Should I send another PR now?

Follow up #5937

OSBotify · 2021-10-23T01:33:18Z

🚀 Deployed to staging by @marcaaron in version: 1.1.8-10 🚀

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

nazam100 · 2021-10-28T16:09:47Z

Accessibility issue: There is an color contrast issue in the chats page.
Color Contrast: Many Pages: Grey icons against white background fail contrast requirements #5506
The visual presentation of meaningful graphical objects or interactive components should have a contrast ratio of at least 3.00:1 with the surrounding background color.
The grey icons (#C6C9CA) on white background (#FFFFFF) have a color contrast ratio of 1.66:1.

nazam100 · 2021-10-28T16:56:34Z

Accessibility issue: WACG failure 1.3.1 info and relationship.
Actual Result: The screen reader is announcing the 'concierge button' name twice.
Expected Result: The screen reader should announce 'concierge button' name only once.

concierge.announces.twice.mp4

nazam100 · 2021-10-28T17:04:21Z

Accessibility issue: WACG failure 2.4.9 Link purpose
Actual Result: The screen reader is announcing the 'Phone button' as clickable
Expected Result: The screen reader should announce the 'Phone button' as phone button

2.4.9.phone.button.and.pin.button.is.not.descriptive.mp4

Other occurrences:

2.4.9.phone.button.and.pin.button.is.not.descriptive.emoji.and.send.button.mp4

nazam100 · 2021-10-28T17:13:30Z

Accessibility issue: WACG failure 2.4.3 Focus order meaning.
Actual Result: The focus is not in the meaningful sequence.
Expected Result: The focus should be in the meaningful sequences

2.4.3.focus.moving.to.the.hidden.content.mp4

Other Occurrences:

2.4.3.focus.is.moving.to.the.dectrative.content.mp4

focus.moves.to.the.hidden.content.mp4

OSBotify · 2021-10-28T20:58:10Z

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

hoangzinh · 2024-02-10T11:05:58Z

src/pages/ReimbursementAccount/EnableStep.js

@@ -83,7 +83,6 @@ class EnableStep extends React.Component {
                            title: translate('workspace.bankAccount.chatWithConcierge'),
                            icon: ChatBubble,
                            onPress: () => {
-                                Navigation.dismissModal();


Comming from #34456, remove dismiss modal here potential causes that bug.

fix: navigation for exisiting report screen

c006880

parasharrajat requested a review from a team as a code owner September 8, 2021 08:18

MelvinBot requested review from marcaaron and removed request for a team September 8, 2021 08:18

marcaaron requested changes Sep 8, 2021

View reviewed changes

marcaaron assigned parasharrajat Sep 8, 2021

refactor

28533f6

parasharrajat commented Sep 14, 2021

View reviewed changes

kadiealexander added the n6-hold label Sep 16, 2021

parasharrajat requested a review from marcaaron October 8, 2021 06:14

This was referenced Oct 10, 2021

Fix: back handler for Drawer #5745

Merged

Close sidebar when going to concierge chat #5779

Merged

Dismiss top modal before navigating to Concierge chat #5911

Merged

parasharrajat added 3 commits October 19, 2021 01:22

Pull latest changes

89ed687

remove extra code

aabd93f

refactor: About Page

5cd8450

marcaaron requested changes Oct 18, 2021

View reviewed changes

parasharrajat added 2 commits October 19, 2021 02:13

removed Extra import

432b542

refactor

9cee8d3

marcaaron removed the n6-hold label Oct 18, 2021

marcaaron self-requested a review October 18, 2021 21:46

marcaaron approved these changes Oct 18, 2021

View reviewed changes

marcaaron merged commit e5fd8d4 into Expensify:main Oct 18, 2021

parasharrajat commented Oct 18, 2021

View reviewed changes

parasharrajat mentioned this pull request Oct 18, 2021

[NO QA] Small Refactor #5937

Merged

OSBotify mentioned this pull request Oct 23, 2021

Deploy Checklist: New Expensify 2021-10-23 #6017

Closed

hoangzinh reviewed Feb 10, 2024

View reviewed changes

hoangzinh mentioned this pull request Feb 10, 2024

[HOLD for payment 2024-02-07] [$500] Workplace - User is unable to return back to homepage after clicking “here” in Concierge chat #34456

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: navigation for exisiting report screen #5131

fix: navigation for exisiting report screen #5131

parasharrajat commented Sep 8, 2021 •

edited

Loading

marcaaron Sep 8, 2021

parasharrajat Sep 8, 2021

parasharrajat Sep 8, 2021

marcaaron Sep 8, 2021

parasharrajat Sep 9, 2021

parasharrajat left a comment

parasharrajat Sep 14, 2021

marcaaron Sep 14, 2021

parasharrajat Sep 14, 2021 •

edited

Loading

marcaaron commented Sep 14, 2021

kadiealexander commented Sep 16, 2021

parasharrajat commented Oct 18, 2021

marcaaron left a comment

marcaaron Oct 18, 2021 •

edited

Loading

parasharrajat Oct 18, 2021

marcaaron Oct 18, 2021

marcaaron Oct 18, 2021

parasharrajat Oct 18, 2021 •

edited

Loading

marcaaron Oct 18, 2021

parasharrajat Oct 18, 2021

marcaaron left a comment

OSBotify commented Oct 18, 2021

parasharrajat Oct 18, 2021 •

edited

Loading

parasharrajat Oct 18, 2021

OSBotify commented Oct 23, 2021

nazam100 commented Oct 28, 2021

nazam100 commented Oct 28, 2021 •

edited

Loading

nazam100 commented Oct 28, 2021 •

edited

Loading

nazam100 commented Oct 28, 2021 •

edited

Loading

OSBotify commented Oct 28, 2021

hoangzinh Feb 10, 2024

fix: navigation for exisiting report screen #5131

fix: navigation for exisiting report screen #5131

Conversation

parasharrajat commented Sep 8, 2021 • edited Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web | Desktop

Mobile Web

iOS

Android

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

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

parasharrajat Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

marcaaron commented Sep 14, 2021

kadiealexander commented Sep 16, 2021

parasharrajat commented Oct 18, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Oct 18, 2021 • 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 Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

OSBotify commented Oct 18, 2021

parasharrajat Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Oct 23, 2021

nazam100 commented Oct 28, 2021

nazam100 commented Oct 28, 2021 • edited Loading

nazam100 commented Oct 28, 2021 • edited Loading

nazam100 commented Oct 28, 2021 • edited Loading

OSBotify commented Oct 28, 2021

Choose a reason for hiding this comment

parasharrajat commented Sep 8, 2021 •

edited

Loading

parasharrajat Sep 14, 2021 •

edited

Loading

marcaaron Oct 18, 2021 •

edited

Loading

parasharrajat Oct 18, 2021 •

edited

Loading

parasharrajat Oct 18, 2021 •

edited

Loading

nazam100 commented Oct 28, 2021 •

edited

Loading

nazam100 commented Oct 28, 2021 •

edited

Loading

nazam100 commented Oct 28, 2021 •

edited

Loading