Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2022-01-19] When you click into a given page in a Workspace and then click back out, the page is still highlighted #7036

Closed
mvtglobally opened this issue Jan 5, 2022 · 20 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Jan 5, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open app
  2. Navigate to workspace
  3. Click any page and navigate back out

Expected Result:

Since user is back to the main page, selected page should not be highlighted

Actual Result:

when you click into a given page in a Workspace and then click back out, the page you clicked into is still highlighted. It remains highlighted even as you over over other pages.

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.25-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Upwork URL: https://www.upwork.com/jobs/~0178e910d1a86795eb

Screen.Recording.2021-12-30.at.2.58.02.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1640905133320000

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @Beamanator (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Beamanator
Copy link
Contributor

I can reproduce too 👍 This seems like a regression, I doubt it was always like this - I'll look around & ask around to see if I can figure out where it came from

@Beamanator
Copy link
Contributor

Ok looks like this started way back here: #4382 - when the workspace pages were full screen. When we added lots more workspace pages & probably made the settings only partial-page here (#5642), we kept the active-page code. This can definitely be done externally, it would be nice if we could get a new contributor to work on this one since it looks easy

@Beamanator Beamanator added the External Added to denote the issue can be worked on by a contributor label Jan 5, 2022
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@thesahindia
Copy link
Member

Proposal

removing the isActive property from menuItems will solve this issue.

const menuItems = [
{
translationKey: 'workspace.common.settings',
icon: Expensicons.Gear,
action: () => Navigation.navigate(ROUTES.getWorkspaceSettingsRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceSettingsRoute(policy.id)),
},
{
translationKey: 'workspace.common.card',
icon: Expensicons.ExpensifyCard,
action: () => Navigation.navigate(ROUTES.getWorkspaceCardRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceCardRoute(policy.id)),
},
{
translationKey: 'workspace.common.reimburse',
icon: Expensicons.Receipt,
action: () => Navigation.navigate(ROUTES.getWorkspaceReimburseRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceReimburseRoute(policy.id)),
},
{
translationKey: 'workspace.common.bills',
icon: Expensicons.Bill,
action: () => Navigation.navigate(ROUTES.getWorkspaceBillsRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceBillsRoute(policy.id)),
},
{
translationKey: 'workspace.common.invoices',
icon: Expensicons.Invoice,
action: () => Navigation.navigate(ROUTES.getWorkspaceInvoicesRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceInvoicesRoute(policy.id)),
},
{
translationKey: 'workspace.common.travel',
icon: Expensicons.Luggage,
action: () => Navigation.navigate(ROUTES.getWorkspaceTravelRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceTravelRoute(policy.id)),
},
{
translationKey: 'workspace.common.members',
icon: Expensicons.Users,
action: () => Navigation.navigate(ROUTES.getWorkspaceMembersRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceMembersRoute(policy.id)),
},
{
translationKey: 'workspace.common.bankAccount',
icon: Expensicons.Bank,
action: () => Navigation.navigate(ROUTES.getWorkspaceBankAccountRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceBankAccountRoute(policy.id)),
},
];

@sobitneupane
Copy link
Contributor

Proposal
Making following change to WorkspaceInitialPage.js will solve the issue.

<MenuItem
                                    key={item.translationKey}
                                    title={this.props.translate(item.translationKey)}
                                    icon={item.icon}
                                    iconRight={item.iconRight}
                                    onPress={() => item.action()}
-                                   wrapperStyle={shouldFocus ? styles.activeComponentBG : undefined}
-                                   focused={shouldFocus}
                                    shouldShowRightIcon
                                />

@botify botify removed the Daily KSv2 label Jan 5, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Jan 5, 2022
@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 5, 2022
@MelvinBot
Copy link

Current assignee @Beamanator is eligible for the Exported assigner, not assigning anyone new.

@trjExpensify
Copy link
Contributor

Exported the job to Upwork here: https://www.upwork.com/jobs/~0178e910d1a86795eb

@Beamanator
Copy link
Contributor

@rushatgabhane would you mind commenting in this issue so I can assign you to help review proposals as a Contributor-plus?

@rushatgabhane
Copy link
Member

@Beamanator Here, thanks

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 5, 2022
@Expensify Expensify deleted a comment from MelvinBot Jan 5, 2022
@Beamanator Beamanator added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 5, 2022
@Beamanator
Copy link
Contributor

Great! Assigned you to help review, @rushatgabhane - hopefully this will be automated starting tomorrow :D

@rushatgabhane
Copy link
Member

@thesahindia What do you plan to do with isFocused which is a required prop?

@thesahindia
Copy link
Member

@rushatgabhane,
I think we can remove both withNavigationFocus and isFocused from that file and do the clean-up since now the focused logic is not needed

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 5, 2022

🎀👀🎀 C+ reviewed
@Beamanator I like @thesahindia's proposal.

Because there is no double side nav drawer for this view, we can't see the focused menu item anyway.
We can remove the focus entirely.

@Beamanator
Copy link
Contributor

I agree! Let's go with @thesahindia 👍

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 5, 2022
@MelvinBot
Copy link

📣 @thesahindia You have been assigned to this job by @Beamanator!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@thesahindia thesahindia mentioned this issue Jan 5, 2022
5 tasks
@trjExpensify
Copy link
Contributor

Hired on Upwork 👍

@botify botify added Weekly KSv2 and removed Weekly KSv2 labels Jan 12, 2022
@botify
Copy link

botify commented Jan 12, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.27-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-01-19. 🎊

@botify botify added the Awaiting Payment Auto-added when associated PR is deployed to production label Jan 12, 2022
@botify botify changed the title When you click into a given page in a Workspace and then click back out, the page is still highlighted [HOLD for payment 2022-01-19] When you click into a given page in a Workspace and then click back out, the page is still highlighted Jan 12, 2022
@trjExpensify
Copy link
Contributor

Settled up on Upwork, @thesahindia. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants