fix: 8329: Resolve fabmenu open multiple time #8692

dharmik · 2022-04-19T17:50:22Z

Details

Fixed Issues

$ 8329

Tests

tests to something like this -

1 Login to New expensify
2 In RHN, tap on the green +
Double tap on new chat
3 Verify that the new chat page opens

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

tests to something like this -

1 Login to New expensify
2 In RHN, tap on the green +
Double tap on new chat
3 Verify that the new chat page opens

Verify that no errors appear in the JS console

Video

iOS

Screen.Recording.2022-04-15.at.10.47.49.AM.mov

android

162147410-a7ee7d7d-de03-4086-ac2c-ed19091aa655.mp4

Macos

Screen.Recording.2022-04-19.at.11.15.18.PM.mov

MWeb

Screen.Recording.2022-04-20.at.11.56.42.PM.mov

Web

Screen.Recording.2022-04-20.at.11.58.54.PM.mov

dharmik · 2022-04-19T17:57:10Z

@rushatgabhane PR updated.

rushatgabhane · 2022-04-19T20:32:26Z

@dharmik please add screenshots for all platforms

And update the tests to something like this -

Login to New expensify
In RHN, tap on the green +
Double tap on new chat
Verify that the new chat page opens

Thanks!

tgolen · 2022-04-19T20:28:39Z

src/pages/home/sidebar/SidebarScreen.js

@@ -58,14 +59,23 @@ class SidebarScreen extends Component {
        Timing.start(CONST.TIMING.SIDEBAR_LOADED, true);

        const routes = lodashGet(this.props.navigation.getState(), 'routes', []);
-        WelcomeAction.show({routes, toggleCreateMenu: this.toggleCreateMenu});
+        WelcomeAction.show({routes, hideCreateMenu: this.hideCreateMenu});
    }

    /**
     * Method called when a Create Menu item is selected.
     */
    onCreateMenuItemSelected() {


If this method only does one thing, which is call another method, then this method should be removed and just call this.hideCreateMenu() instead.

@tgolen You mean to say we need to make the change like this?

- WelcomeAction.show({routes, hideCreateMenu: this.hideCreateMenu}); + this.hideCreateMenu();

If yes? WelcomeActions.js we are managing this condition and call method in if statement.

if (!Policy.isAdminOfFreePolicy(allPolicies) && !isDisplayingWorkspaceRoute) { hideCreateMenu(); }

Any thoughts on it?

@dharmik, onCreateMenuItemSelected() just calls hideCreateMenu().

So you need to

Remove onCreateMenuItemSelected(), and

Replace all calls to it with hideCreateMenu()

Let me know if you have any questions.

@rushatgabhane Resolved

@rushatgabhane @tgolen Can you please share your opinion on it? #8692 (comment)

Can you please share your opinion on it? #8692 (comment)

@dharmik I'm sorry, I don't understand. I thought this conversation was resolved.
Only action required by you is to address #8692 (comment)

Please let me know if you have any questions, thanks so much!

tgolen · 2022-04-19T20:29:53Z

src/libs/actions/WelcomeActions.js

@@ -75,7 +75,7 @@ function show({routes, toggleCreateMenu}) {
        // It's also possible that we already have a workspace policy. In either case we will not toggle the menu but do still want to set the NVP in this case


This comment needs to be updated now (since it specifically mentions toggling)

@dharmik ig you missed this

@rushatgabhane PR updated.

dharmik · 2022-04-20T18:41:00Z

@rushatgabhane I updated videos in PR description is that okay?

dharmik · 2022-04-22T20:42:47Z

@rushatgabhane Can you please recheck this PR & let me know if I miss something?

rushatgabhane

@tgolen Looks good!

#### 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.

rushatgabhane · 2022-04-22T20:54:47Z

@dharmik sorry for the delay, you just need to address this comment. Thanks so much!

dharmik · 2022-04-23T17:02:33Z

@rushatgabhane

#8692

I added all videos in PR description

rushatgabhane · 2022-04-23T17:14:55Z

@dharmik the tests aren't clear, so please address that too. My comment here was a suggestion for that

dharmik · 2022-04-23T17:21:40Z

if in those video testes are not clear so in screenshot how clear?

Appreciate your suggestion

@rushatgabhane

rushatgabhane · 2022-04-23T17:39:00Z

Ohh I see. Forget the video completely. As per the suggestions here update these tests in the PR description 👇

dharmik · 2022-04-23T19:12:56Z

PR description updated @rushatgabhane

rushatgabhane · 2022-04-24T22:39:52Z

Btw, PR is approved!

OSBotify · 2022-04-25T18:01:39Z

✋ 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-04-25T23:42:07Z

🚀 Deployed to staging by @tgolen in version: 1.1.57-0 🚀

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

marcaaron · 2022-04-28T20:28:34Z

Sorry if I'm missing some context but doesn't this PR break the "automatic" opening on the create menu for a first time user sign in since we are only "hiding" it now?

I find it kind of weird that we are calling WelcomeAction.show() but it doesn't "show" anything and either navigates somewhere or calls a hide() method.

marcaaron · 2022-04-28T20:29:37Z

App/src/libs/actions/WelcomeActions.js

Lines 43 to 81 in db6fab7

    
           /** 
        
            * Shows a welcome action on first login 
        
            * 
        
            * @param {Object} params 
        
            * @param {Object} params.routes 
        
            * @param {Function} params.hideCreateMenu 
        
            */ 
        
           function show({routes, hideCreateMenu}) { 
        
               // NOTE: This setTimeout is required due to a bug in react-navigation where modals do not display properly in a drawerContent 
        
               // This is a short-term workaround, see this issue for updates on a long-term solution: https://github.com/Expensify/App/issues/5296 
        
               setTimeout(() => { 
        
                   if (!isFirstTimeNewExpensifyUser) { 
        
                       return; 
        
                   } 
        
                   // Set the NVP back to false so we don't automatically run welcome actions again 
        
                   NameValuePair.set(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, false, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER); 
        
                   // We want to display the Workspace chat first since that means a user is already in a Workspace and doesn't need to create another one 
        
                   const workspaceChatReport = _.find(allReports, report => ReportUtils.isPolicyExpenseChat(report)); 
        
                   if (workspaceChatReport) { 
        
                       Navigation.navigate(ROUTES.getReportRoute(workspaceChatReport.reportID)); 
        
                       return; 
        
                   } 
        
                   // If we are rendering the SidebarScreen at the same time as a workspace route that means we've already created a workspace via workspace/new and should not open the global 
        
                   // create menu right now. 
        
                   const topRouteName = lodashGet(_.last(routes), 'name', ''); 
        
                   const loginWithShortLivedTokenRoute = _.find(routes, route => route.name === 'LogInWithShortLivedToken'); 
        
                   const exitingToWorkspaceRoute = lodashGet(loginWithShortLivedTokenRoute, 'params.exitTo', '') === 'workspace/new'; 
        
                   const isDisplayingWorkspaceRoute = topRouteName.toLowerCase().includes('workspace') || exitingToWorkspaceRoute; 
        
                   // It's also possible that we already have a workspace policy. In either case we will not hide the menu but do still want to set the NVP in this case 
        
                   // since the user does not need to create a workspace. 
        
                   if (!Policy.isAdminOfFreePolicy(allPolicies) && !isDisplayingWorkspaceRoute) { 
        
                       hideCreateMenu(); 
        
                   } 
        
               }, 1500); 
        
           }

Anyone can tell us where in this function we "show" something?

marcaaron · 2022-04-28T20:41:30Z

Thinking maybe it should have been showCreateMenu() and we just got mixed up because it wasn't clear if "toggle" meant "hide" or "show". But it means "show" right?

marcaaron · 2022-04-28T20:42:35Z

(context: While looking into the issue here realized the code no longer makes sense since we're not showing anything)

tgolen · 2022-04-28T21:13:35Z

Those are pretty good questions. I actually think there is quite a bit about the welcome actions code that is an anti-pattern (like passing callback functions into the action file, the use of the setTimeout(), etc.). I would say there aren't really any good answers, and I would be in favor of just doing a bit of an over-haul and cleanup to this code.

marcaaron · 2022-04-28T22:07:10Z

I agree, but this PR still broke existing behavior. We are calling a function that doesn't show anything and won't hide something because nothing is shown in the first place.

tgolen · 2022-04-28T22:13:09Z

Yeah, so in that case, it's definitely a regression that needs fixed regardless. You are right.

dharmik · 2022-04-29T14:06:04Z

@marcaaron : I realize it, thanks for point it out.

File: In src/libs/actions/WelcomeActions.js we only display it to Show menu item first time for isFirstTimeNewExpensifyUser = true

Changes:
File: src/pages/home/sidebar/SidebarScreen.js

-        WelcomeAction.show({routes, hideCreateMenu: this.hideCreateMenu});
+        WelcomeAction.show({routes, showCreateMenu: this.showCreateMenu});

File: src/libs/actions/WelcomeActions.js

- * @param {Function} params.hideCreateMenu
+ * @param {Function} params.showCreateMenu
  */
-function show({routes, hideCreateMenu}) {
+function show({routes, showCreateMenu}) {

         if (!Policy.isAdminOfFreePolicy(allPolicies) && !isDisplayingWorkspaceRoute) {
-            hideCreateMenu();
+            showCreateMenu();

@marcaaron Is it okay?

OSBotify · 2022-05-13T16:14:41Z

🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀

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

fix: 8329: Resolve fabmenu open multiple time

fb511db

dharmik requested a review from a team as a code owner April 19, 2022 17:50

melvin-bot bot requested review from rushatgabhane and tgolen and removed request for a team April 19, 2022 17:50

tgolen requested changes Apr 19, 2022

View reviewed changes

Resolve comment: onCreateMenuItemSelected from sidebarscreen.

375aad5

As per comment change the message in WelcomeActions.

1978bf7

rushatgabhane approved these changes Apr 22, 2022

View reviewed changes

tgolen approved these changes Apr 25, 2022

View reviewed changes

tgolen merged commit 16b130b into Expensify:main Apr 25, 2022

francoisl mentioned this pull request Apr 25, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

marcaaron mentioned this pull request Apr 28, 2022

Remove setTimeout(). Fix behavior of create menu. #8827

Merged

45 tasks

This was referenced May 2, 2022

[low] Chrome + Jaws: New chat(+) dropdown expanded/collapsed state is not announced. #8667

Closed

[low] Chrome + Talkback: Screen reader announces extra information announced on dropdown. #8620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: 8329: Resolve fabmenu open multiple time #8692

fix: 8329: Resolve fabmenu open multiple time #8692

dharmik commented Apr 19, 2022 •

edited

Loading

dharmik commented Apr 19, 2022

rushatgabhane commented Apr 19, 2022 •

edited

Loading

tgolen Apr 19, 2022

dharmik Apr 20, 2022

rushatgabhane Apr 20, 2022

dharmik Apr 20, 2022

dharmik Apr 20, 2022

rushatgabhane Apr 21, 2022 •

edited

Loading

tgolen Apr 19, 2022

rushatgabhane Apr 20, 2022

dharmik Apr 21, 2022

dharmik commented Apr 20, 2022

dharmik commented Apr 22, 2022

rushatgabhane left a comment

rushatgabhane commented Apr 22, 2022 •

edited

Loading

dharmik commented Apr 23, 2022 •

edited

Loading

rushatgabhane commented Apr 23, 2022

dharmik commented Apr 23, 2022

rushatgabhane commented Apr 23, 2022

dharmik commented Apr 23, 2022

rushatgabhane commented Apr 24, 2022

OSBotify commented Apr 25, 2022

OSBotify commented Apr 25, 2022

marcaaron commented Apr 28, 2022

marcaaron commented Apr 28, 2022

marcaaron commented Apr 28, 2022

marcaaron commented Apr 28, 2022

tgolen commented Apr 28, 2022

marcaaron commented Apr 28, 2022

tgolen commented Apr 28, 2022

dharmik commented Apr 29, 2022 •

edited

Loading

OSBotify commented May 13, 2022

		@@ -75,7 +75,7 @@ function show({routes, toggleCreateMenu}) {
		// It's also possible that we already have a workspace policy. In either case we will not toggle the menu but do still want to set the NVP in this case

fix: 8329: Resolve fabmenu open multiple time #8692

fix: 8329: Resolve fabmenu open multiple time #8692

Conversation

dharmik commented Apr 19, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Video

dharmik commented Apr 19, 2022

rushatgabhane commented Apr 19, 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

Choose a reason for hiding this comment

rushatgabhane Apr 21, 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

dharmik commented Apr 20, 2022

dharmik commented Apr 22, 2022

rushatgabhane left a comment

Choose a reason for hiding this comment

#### PR Reviewer Checklist

rushatgabhane commented Apr 22, 2022 • edited Loading

dharmik commented Apr 23, 2022 • edited Loading

rushatgabhane commented Apr 23, 2022

dharmik commented Apr 23, 2022

rushatgabhane commented Apr 23, 2022

dharmik commented Apr 23, 2022

rushatgabhane commented Apr 24, 2022

OSBotify commented Apr 25, 2022

OSBotify commented Apr 25, 2022

marcaaron commented Apr 28, 2022

marcaaron commented Apr 28, 2022

marcaaron commented Apr 28, 2022

marcaaron commented Apr 28, 2022

tgolen commented Apr 28, 2022

marcaaron commented Apr 28, 2022

tgolen commented Apr 28, 2022

dharmik commented Apr 29, 2022 • edited Loading

OSBotify commented May 13, 2022

dharmik commented Apr 19, 2022 •

edited

Loading

rushatgabhane commented Apr 19, 2022 •

edited

Loading

rushatgabhane Apr 21, 2022 •

edited

Loading

rushatgabhane commented Apr 22, 2022 •

edited

Loading

dharmik commented Apr 23, 2022 •

edited

Loading

dharmik commented Apr 29, 2022 •

edited

Loading