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

The hover effect animation is not functioning properly on the Bank Account in workspace settings #35839

Closed
1 of 6 tasks
lanitochka17 opened this issue Feb 5, 2024 · 13 comments
Assignees

Comments

@lanitochka17
Copy link

lanitochka17 commented Feb 5, 2024

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


Version Number: 1.4.36-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): abebemiherat@gmail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Navigate to Settings > Workspace > Bank Account
  2. Click on it; the modal opens on the right side
  3. The hover effect animation should be centered on the Bank Account section

Expected Result:

The hover effect animation should be centered on the Bank Account section

Actual Result:

The hover effect animation is not centered on the Bank Account section

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6367956_1707149094676.BA.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Feb 5, 2024
Copy link
Contributor

github-actions bot commented Feb 5, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Feb 5, 2024

Triggered auto assignment to @rlinoz (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@mountiny mountiny removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Feb 5, 2024
@mountiny mountiny assigned mountiny and unassigned rlinoz Feb 5, 2024
@mountiny mountiny added the Daily KSv2 label Feb 5, 2024
@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

Taking over, related to ideal nav.

@esh-g
Copy link
Contributor

esh-g commented Feb 5, 2024

Proposal

Please re-state the problem we are trying to fix

The hover effect animation is not functioning properly on the Bank Account in workspace settings

What is the root cause of the problem?

We are checking if the Menuitem is pressed based on whether the item route name is present in the route, but for bankAccount we are not passing any routeName

focused={activeRoute && activeRoute.startsWith(item.routeName)}

What changes should be made to fix this?

  1. We should pass routeName property for bankAccount item in WorkspaceInitialPage

    {
    translationKey: 'workspace.common.bankAccount',
    icon: Expensicons.Bank,
    action: () =>
    policy.outputCurrency === CONST.CURRENCY.USD
    ? singleExecution(waitForNavigate(() => ReimbursementAccount.navigateToBankAccountRoute(policy.id, Navigation.getActiveRouteWithoutParams())))()
    : setIsCurrencyModalOpen(true),
    brickRoadIndicator: !_.isEmpty(props.reimbursementAccount.errors) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '',
    },

    We should pass it as SCREENS.REIMBURSEMENT_ACCOUNT.

  2. We are only getting the activeRoute for CentralPaneNavigator whereas the bank account page is in RHP. Therefore we should modify the BottomTabNavigator to pass both active routes for both RHP and CentralPane here:

    function BottomTabNavigator() {
    const activeRoute = useNavigationState(getTopmostCentralPaneRoute);
    return (
    <ActiveRouteContext.Provider value={activeRoute?.name ?? ''}>

    This means the activeRoute will be changed from just string to:

type ActiveRouteType = {
  centralPaneActiveRoute: string;
  rightModalActiveRoute: string;
}
Screenshot 2024-02-05 at 11 32 03 PM
  1. Instead of just checking for centralPane we can add the check for RHP here:
    focused={activeRoute && activeRoute.startsWith(item.routeName)}

    by creating this function to check if focused and pass it to focused prop:
const isFocusedRoute = (routeName) => {
        if (activeRoute.centralPaneActiveRoute.startsWith(routeName) && activeRoute.rightModalActiveRoute === NAVIGATORS.CENTRAL_PANE_NAVIGATOR) {
            return true;
        } else if (activeRoute.rightModalActiveRoute.startsWith(routeName)) {
            return true;
        }
        return false;
    };

Result

Screen.Recording.2024-02-05.at.11.41.47.PM.mov

@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

@trjExpensify Curious for your opinion on this one, I feel like we should eventually update the bank account to have its own page in the main pane same as all the other settings pages.

If we would do that, this change would not be necessary

@trjExpensify
Copy link
Contributor

trjExpensify commented Feb 5, 2024

Yeah, interesting one. We could move the Bank account page to the main pane like we have the others. Clicking the rows in the card open the modal in the RHP.

That said, with Simplified Collect: Workflows when we implement the new re-designed workspace editor, it'll actually be RHP again:

image

So perhaps the decision to do nothing was the right one for the time being. It's a release1 initiative, so we'd be living with this for a month or so. CC: @luacmartins @JmillsExpensify

@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

@trjExpensify alright I see we will not have any case when the Bank account would be in the LHP. then I can see us doing nothing here

@esh-g
Copy link
Contributor

esh-g commented Feb 5, 2024

Does that mean the bank account page will remain in RHP or LHP? We also have the case where we have a flow from central navigator to the bank account page in RHP, so this can't be moved to central pane at least

Screen.Recording.2024-02-06.at.1.04.30.AM.mov

@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

That means there will be no Bank Account option in LHP

@trjExpensify
Copy link
Contributor

We also have the case where we have a flow from central navigator to the bank account page in RHP, so this can't be moved to central pane at least

Nah, all of those weird redirects will go away with the redesigned workspace editor as those pages get replaced.

@trjExpensify
Copy link
Contributor

@trjExpensify alright I see we will not have any case when the Bank account would be in the LHP.

Nah, there won't be a Bank account menu item in the LHN if that's the question. Further, the Connect bank account > push row in the main pane of Workflows will open the Connect bank account page in the RHP.

@mountiny mountiny added Monthly KSv2 and removed Daily KSv2 labels Feb 8, 2024
@mountiny
Copy link
Contributor

mountiny commented Feb 8, 2024

making this monthly and will confirm once we update this flow

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
@mountiny
Copy link
Contributor

This button will be removed from the LHP so I am going to close this issue

@melvin-bot melvin-bot bot removed the Overdue label Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Status: Release 1: Ideal Nav & Collect Simplfied Profile, Members, Categories, Workflows (approvals)
Development

No branches or pull requests

5 participants