[HOLD for payment 2024-11-20] [$125] Don't show two creation options at the same time in the workspace switcher

JmillsExpensify commented 1 week ago

Problem: When we prompt users to create a new workspace in the workspace switcher, they become confused by two options that do the same thing, which leads to frustration.

Solution: Let's simplify the decisions we're asking users to take when creating a workspace, and in particular, let's not add creation options that do the same thing and compete with our "green brick road" pattern.

All in all, our solution will look like this:

Don't show the plus button when we show the empty state promoting workspace creation.

For clarity, this means that we should show the small plus button only when the empty state doesn't show. (Related, the empty state should only show when someone doesn't have an existing group workspace that they're a member of). File

Issue Owner

Current Issue Owner: @JmillsExpensify

melvin-bot[bot] commented 1 week ago

Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

JmillsExpensify commented 1 week ago

Let me know if you have any questions on this issues, happy to clarify.

NJ-2020 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-05 11:54:40 UTC.

Proposal

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

Don't show two creation options at the same time in the workspace switcher

What is the root cause of that problem?

We did not hide the option if the user does not have workspaces or member group of a workspace https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx#L31-L33

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

We should check if the user has a workspace or a member of other group workspaces then we will show the option inside the workspace switcher page

const [allPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);

{Policy.hasActiveChatEnabledPolicies(Object.values(allPolicies ?? {})) && (
    <Tooltip text={translate('workspace.new.newWorkspace')}>
        <PressableWithFeedback
            accessibilityLabel={translate('workspace.new.newWorkspace')}
            role={CONST.ROLE.BUTTON}
            onPress={() => {
                const activeRoute = Navigation.getActiveRouteWithoutParams();
                interceptAnonymousUser(() => App.createWorkspaceWithPolicyDraftAndNavigateToIt('', '', false, false, activeRoute));
            }}
        >
            {({hovered}) => (
                <Icon
                    src={Expensicons.Plus}
                    width={12}
                    height={12}
                    additionalStyles={[styles.buttonDefaultBG, styles.borderRadiusNormal, styles.p2, hovered && styles.buttonHoveredBG]}
                    fill={theme.icon}
                />
            )}
        </PressableWithFeedback>
    </Tooltip>
)}

In this issue we decided to return true for hasActiveChatEnabledPolicies if the user is member of other group workspaces

What alternative solutions did you explore? (Optional)

JmillsExpensify commented 1 week ago

Following on that proposal, please check this related issue as it's important that we stay consistent with it. In short, we shouldn't be showing the empty state as long as someone is a member of other group workspaces (whether or not they are policy expense chats enabled).

twilight2294 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-05 12:05:03 UTC.

Proposal

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

Don't show two creation options at the same time in the workspace switcher

What is the root cause of that problem?

We do not hide the Plus icon for empty state view:

https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx#L31-L33

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

We need to pass shouldShowCreateWorkspace: https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/index.tsx#L149

to the below component:

https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/index.tsx#L186

<WorkspacesSectionHeader shouldShowCreateWorkspace={!shouldShowCreateWorkspace} />

And then in WorkspacesSectionHeader:

(shouldShowCreateWorkspace && <Tooltip text={translate('workspace.new.newWorkspace')}>
                <PressableWithFeedback
                    accessibilityLabel={translate('workspace.new.newWorkspace')}

We already pass shouldShowCreateWorkspace to show the empty content screen: https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/index.tsx#L196

What alternative solutions did you explore? (Optional)

nyomanjyotisa commented 1 week ago

Proposal

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

Don't show two creation options at the same time in the workspace switcher

What is the root cause of that problem?

Changes request

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

Accept shouldShowCreateWorkspace on WorkspacesSectionHeader https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx#L16

And only show the + icon if shouldShowCreateWorkspace is true https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx#L31

            {shouldShowCreateWorkspace && (
                <Tooltip text={translate('workspace.new.newWorkspace')}>
                     ...
                </Tooltip>
            )}

Pass shouldShowCreateWorkspace, but we should use the opposite of the existing shouldShowCreateWorkspace variable, so we should pass !shouldShowCreateWorkspace https://github.com/Expensify/App/blob/3d0f309b5ebc4f5bfd1279949c6eddca663e42a4/src/pages/WorkspaceSwitcherPage/index.tsx#L186

                    <WorkspacesSectionHeader
                        shouldShowCreateWorkspace={!shouldShowCreateWorkspace}
                    />

Result

What alternative solutions did you explore? (Optional)

twilight2294 commented 1 week ago

Updated proposal

Updated the proposal to add an extra point and pass the prop to main component

NJ-2020 commented 1 week ago

Proposal Updated

brunovjk commented 1 week ago

I’ll review the proposals shortly.

brunovjk commented 1 week ago

Thank you, @NJ-2020, for proposal 1, but I don’t think the solution is suitable here.

Between proposals 2 and 3, both suggest a correct solution of passing shouldShowCreateWorkspace as a prop to WorkspacesSectionHeader. @Twilight2294 (proposal 2) submitted first and identified the correct overall solution. @Nyomanjyotisa (proposal 3) contributed a key detail—setting shouldShowCreateWorkspace to !shouldShowCreateWorkspace when passing it. I’m not sure if one used information from the other, but I think this detail could be noticed later anyway, like other details (e.g., prop type) we must check in PR. IMO the core of the solution was pointed out by proposal 2 first.

I think we can go with @Twilight2294's proposal. However, both posted the proposal almost at the same time, difficult decision, I ask for your help with this call hehe

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @MariaHCD, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

nyomanjyotisa commented 1 week ago

@brunovjk @MariaHCD I believe my proposal is the first with the correct solution

Timestamp:

2024-11-05T12:03:30Z - Proposal 2 posted
2024-11-05T12:04:58Z - Proposal 3 posted
2024-11-05T12:07:55Z - Proposal 2 solution updated

Btw to be honest I started writing my proposal before proposal 2 was posted, and don't use any information from proposal 2

twilight2294 commented 1 week ago

I think we can go with @twilight2294's https://github.com/Expensify/App/issues/52030#issuecomment-2456991192.

@brunovjk , yes i was the first one to give the correct approach to the bug, so my solution is valid in this case :)

MariaHCD commented 1 week ago

I agree with @brunovjk's decision here 👍🏼 The two proposals were posted at almost the same time and propose the same fix.

Let's proceed with @twilight2294 for this one.

Thank you for the contribution, @nyomanjyotisa!

melvin-bot[bot] commented 2 days ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 days ago

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

https://github.com/Expensify/App/pull/52254

If no regressions arise, payment will be issued on 2024-11-20. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@brunovjk requires payment (Needs manual offer from BZ)
@twilight2294 requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 2 days ago

@brunovjk @JmillsExpensify @brunovjk The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

Expensify / App