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

Background changes to workspace when changing the workspace name #38436

Closed
5 of 6 tasks
kbecciv opened this issue Mar 16, 2024 · 7 comments
Closed
5 of 6 tasks

Background changes to workspace when changing the workspace name #38436

kbecciv opened this issue Mar 16, 2024 · 7 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@kbecciv
Copy link

kbecciv commented Mar 16, 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.53-2
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Team

Action Performed:

  1. Create a workspace from "Choose a workspace"
  2. Now click on workspace name
  3. Notice background changed

Expected Result:

The background should not change when RHN open

Actual Result:

Background changes to workspace when changing the workspace name

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

Bug6415760_1710574375920.2024-03-16_12-18-33.mp4

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 16, 2024
Copy link

melvin-bot bot commented Mar 16, 2024

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@kbecciv
Copy link
Author

kbecciv commented Mar 16, 2024

@miljakljajic I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@ShridharGoel
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace lists page opens up when trying to update workspace name, currency etc after creating a new workspace.

What is the root cause of that problem?

This logic is unnecessarily pushing the workspaces list page in the central pane.

// When we open a screen in RHP from FullScreenNavigator, we need to add the appropriate screen in CentralPane.
// Then, when we close FullScreenNavigator, we will be redirected to the correct page in CentralPane.
if (matchingRootRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) {
routes.push(createCentralPaneNavigator({name: SCREENS.SETTINGS.WORKSPACES}));
}

What changes do you think we should make in order to solve the problem?

There's no need of the above logic, we can simply remove it and it will solve the navigation issue. Same issue can be seen at multiple other places which will also get resolved.

@ghost
Copy link

ghost commented Mar 17, 2024

This is the regression caused by this PR. Commented on the PR as well

@allgandalf
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

We have a logic added from #37421 Ideal Nav PR which sets the Central Pane to Workspace settings page

// When we open a screen in RHP from FullScreenNavigator, we need to add the appropriate screen in CentralPane.
// Then, when we close FullScreenNavigator, we will be redirected to the correct page in CentralPane.
if (matchingRootRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) {
routes.push(createCentralPaneNavigator({name: SCREENS.SETTINGS.WORKSPACES}));
}

This was done to address the condition where RHP was opened from FullScreenNavigator, to adjust to the correct page of central pane.

But over here we only consider the condition when RHP was opened from LHN and not the condition where RHP is opened from Central Pane.

What changes do you think we should make in order to solve the problem?

We need to consider an additional Condition to check if the RHP was opened from LHN and then only Set the Central Pane to workspace settings Page:

- if (matchingRootRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) {
+ if ((matchingRootRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) && isRHPScreenOpenedFromLHN) {
                routes.push(createCentralPaneNavigator({name: SCREENS.SETTINGS.WORKSPACES}));
            }          

What alternative solutions did you explore? (Optional)

N/A

@allgandalf
Copy link
Contributor

Dropping proposal here too.

But i think this issue is related to #38420 and should be either closed or put on hold

@melvin-bot melvin-bot bot added the Overdue label Mar 18, 2024
@miljakljajic
Copy link
Contributor

I agree that we can close in favour of #38420

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2
Projects
None yet
Development

No branches or pull requests

4 participants