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 2024-11-28] [$250] Remove plus icon in the workspace switcher when you already have at least one workspace #52409

Closed
JmillsExpensify opened this issue Nov 12, 2024 · 32 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Nov 12, 2024

We originally added the ability to create as many workspaces as you want via the workspace switcher. Unfortunately this is creating confusion and duplicate workspaces, and it's ultimately a whack-a-mole situation given that we already have other workspace creation flows via Track, Global Create and Settings.

As a result, in an effort to simply this particular flow, moving forward we will limit the workspace switcher to exactly two things:

  1. Creating your first workspace. As a reminder, we already show an empty state to create your first workspace, as long as you're not part of any existing workspaces. (The empty state for this case looks like this). This prevents accidental duplicate creation while still promoting that you create at least one.
    File

  2. Focus the workspace switcher on...switching between workspaces. As a result, we'll remove the plus icon when you have one or more workspaces (highlighted in the red box below). This ensures that our multi-workspace creation flow uniformly happens via Settings, and keeps the workspace switcher focused on switching.
    CleanShot 2024-11-12 at 17 15 11@2x

Issue OwnerCurrent Issue Owner: @JmillsExpensify
@JmillsExpensify JmillsExpensify added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 12, 2024
@JmillsExpensify JmillsExpensify self-assigned this Nov 12, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 12, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

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

Copy link

melvin-bot bot commented Nov 12, 2024

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

@cretadn22
Copy link
Contributor

cretadn22 commented Nov 12, 2024

Proposal

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

Remove plus icon in the workspace switcher when you already have at least one workspace

What is the root cause of that problem?

New feature

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

In WorkspacesSectionHeader, we need to remove shouldShowCreateWorkspaceIcon prop and remove the New workspace button

What alternative solutions did you explore? (Optional)

We can remove WorkspacesSectionHeader component add this code to WorkspaceSwitcherPage directly

<View>
<Text
style={styles.label}
color={theme.textSupporting}
>
{translate('common.workspaces')}
</Text>
</View>

@nkdengineer
Copy link
Contributor

Proposal

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

Remove plus icon in the workspace switcher when you already have at least one workspace

What is the root cause of that problem?

New feature

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

Since we don't want to display the plus icon any more, we can remove this here and remove the shouldShowCreateWorkspaceIcon prop

{shouldShowCreateWorkspaceIcon && (

What alternative solutions did you explore? (Optional)

@Nodebrute
Copy link
Contributor

Nodebrute commented Nov 12, 2024

Edited by proposal-police: This proposal was edited at 2024-11-12 16:28:26 UTC.

Proposal

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

Remove plus icon in the workspace switcher when you already have at least one workspace

What is the root cause of that problem?

Feature request

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

We can remove this code block

{shouldShowCreateWorkspaceIcon && (
<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>
)}

Note:Other cleanup can be done in the pr(for example removing unnecessary props)

What alternative solutions did you explore? (Optional)

We can also pass false here

<WorkspacesSectionHeader shouldShowCreateWorkspaceIcon={!shouldShowCreateWorkspace} />

@FitseTLT
Copy link
Contributor

Proposal

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

Remove plus icon in the workspace switcher when you already have at least one workspace

What is the root cause of that problem?

We show the button if the workspaces list is non-empty

const shouldShowCreateWorkspace = usersWorkspaces.length === 0;

<WorkspacesSectionHeader shouldShowCreateWorkspaceIcon={!shouldShowCreateWorkspace} />

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

We should show it when the workspaces list is zero

                    <WorkspacesSectionHeader shouldShowCreateWorkspaceIcon={shouldShowCreateWorkspace} />

What alternative solutions did you explore? (Optional)

@JmillsExpensify JmillsExpensify changed the title Remove plus icon in the workspace switcher when you already have at least one workspace [$250] Remove plus icon in the workspace switcher when you already have at least one workspace Nov 12, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

⚠️ 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.

@twilight2294
Copy link
Contributor

Proposal

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

Remove plus icon in the workspace switcher when you already have at least one workspace

What is the root cause of that problem?

Feature request,

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

Now we do not need the WorkspacesSectionHeader component anymore as we are only displaying the text and not icon, so we should delete the component WorkspacesSectionHeader altogether and then update the WorkspaceSwitcherPage to only display the text as follows:

--- a/src/pages/WorkspaceSwitcherPage/index.tsx
+++ b/src/pages/WorkspaceSwitcherPage/index.tsx
@@ -183,7 +183,16 @@ function WorkspaceSwitcherPage() {
                         pressableStyle={styles.flexRow}
                         shouldSyncFocus={false}
                     />
-                    <WorkspacesSectionHeader shouldShowCreateWorkspaceIcon={!shouldShowCreateWorkspace} />
+                    <View style={[styles.ph5, styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter, styles.mv2]}>
+                        <View>
+                            <Text
+                                style={styles.label}
+                                color={theme.textSupporting}
+                            >
+                                {translate('common.workspaces')}
+                            </Text>
+                        </View>
+                    </View>
                     <SelectionList<WorkspaceListItem>
                         ListItem={UserListItem}
                         sections={sections}

What alternative solutions did you explore? (Optional)

@JmillsExpensify
Copy link
Author

@suneox We've got a good amount of proposals on this issue, so let's try to prioritize getting through the first round of proposal review tomorrow. Thank you!

@Krishna2323
Copy link
Contributor

@JmillsExpensify, isn't this a dupe of #52030?

@suneox
Copy link
Contributor

suneox commented Nov 12, 2024

Since WorkspacesSectionHeader is only used on WorkspaceSwitcherPage and now it only contains a text header after removing the plus add icon so @twilight2294 solution remove WorkspaceSwitcherPage LGTM

We can go with @cretadn22 alternative proposal

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Nov 12, 2024

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

@cretadn22
Copy link
Contributor

@suneox I’ve already outlined this approach in my alternative solution. Could you please take another look?

@suneox
Copy link
Contributor

suneox commented Nov 13, 2024

@suneox I’ve already outlined this approach in my alternative solution. Could you please take another look?

@cretadn22 Your alternative solution is missing container style

@cretadn22
Copy link
Contributor

cretadn22 commented Nov 13, 2024

@suneox This is a minor issue that can be addressed during the PR phase. I believe the proposal only needs to present the idea with a draft implementation to elaborate on it. I also feel it’s unfair when my proposal isn't selected because of the lack of minor styles in the proposal

@suneox
Copy link
Contributor

suneox commented Nov 13, 2024

Based on contributingGuides item 6 in Propose a solution for the job about

- Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The *difference* should be important, meaningful or considerable.

I'd like to update selected proposal to @cretadn22 alternative proposal. Thanks for all proposal

cc: @marcochavezf

@twilight2294
Copy link
Contributor

twilight2294 commented Nov 13, 2024

@suneox this is not fair, with @cretadn22 if you do not apply the container style there would be alignment issues and the text would look weird, this should had been considered before writing the proposal :)

can you please apply the proposal from @cretadn22 and check yourself ? They posted incomplete proposal in the rush to post first

The difference should be important, meaningful or considerable.

The difference is indeed important as the style doesn't match the expected design if we do not also use container styles

In @cretadn22 's solution they mention that We can remove WorkspacesSectionHeader component add this code to WorkspaceSwitcherPage directly

When we implement @cretadn22 solution, the result we get is:

Screenshot 2024-11-13 at 4 00 07 PM

This is not at all expected @suneox @marcochavezf

The code they suggested didn't include the container style, which is important as without container style there are style issues in the page.

c.c. @marcochavezf

@Nodebrute
Copy link
Contributor

Nodebrute commented Nov 13, 2024

Just my two cents here: I believe this is a straightforward feature request, and minor styling adjustments can be easily handled in the PR itself. A proposal doesn’t need to include every small detail, and adding these minor changes to previous proposals might be unnecessary. I think this selection seems fair.

@garrettmknight garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 14, 2024
@garrettmknight garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 14, 2024
@twilight2294
Copy link
Contributor

Thanks, indeed hard luck but looking to contribute further

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 15, 2024
@JmillsExpensify
Copy link
Author

@JmillsExpensify, isn't this a dupe of #52030?

Quickly coming back to this comment, the two issues are different:

We probably could have done them in the same PR, though these ended up being decided in separate convos, at separate intervals. Thus, we have two issues.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 21, 2024
@melvin-bot melvin-bot bot changed the title [$250] Remove plus icon in the workspace switcher when you already have at least one workspace [HOLD for payment 2024-11-28] [$250] Remove plus icon in the workspace switcher when you already have at least one workspace Nov 21, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 21, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

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

Copy link

melvin-bot bot commented Nov 21, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.64-4 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 2024-11-28. 🎊

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

  • @suneox requires payment (Needs manual offer from BZ)
  • @cretadn22 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Nov 21, 2024

@suneox @JmillsExpensify @suneox 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]

@suneox
Copy link
Contributor

suneox commented Nov 25, 2024

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other: Improvement

Where bug was reported:

  • 2a. Reported on production
  • 2b. Reported on staging (deploy blocker)
  • 2c. Reported on both staging and production
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment: This is an improvement, so we don’t have an offending PR.

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion: N/A

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
    N/A: It isn't an impactful bug

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 27, 2024
Copy link

melvin-bot bot commented Nov 28, 2024

Payment Summary

Upwork Job

  • ROLE: @suneox paid $(AMOUNT) via Upwork (LINK)
  • ROLE: @cretadn22 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@JmillsExpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@JmillsExpensify
Copy link
Author

Payment summary:

@suneox
Copy link
Contributor

suneox commented Nov 28, 2024

We’re still waiting for the offer

@JmillsExpensify
Copy link
Author

Offers sent

@suneox
Copy link
Contributor

suneox commented Nov 29, 2024

Thank you i've accepted the offer

@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2024
Copy link

melvin-bot bot commented Dec 3, 2024

@JmillsExpensify, @marcochavezf, @suneox, @cretadn22 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@JmillsExpensify
Copy link
Author

All contributors paid out and this issue removes functionality so I'm good with the checklist items above.

@melvin-bot melvin-bot bot removed the Overdue label Dec 3, 2024
@github-project-automation github-project-automation bot moved this from Bugs and Follow Up Issues to Done in [#whatsnext] #expense Dec 3, 2024
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 Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

9 participants