Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

[$250] Remove plus icon in the workspace switcher when you already have at least one workspace #52409

Open JmillsExpensify opened 1 day ago

JmillsExpensify commented 1 day ago

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: @suneox
melvin-bot[bot] commented 1 day ago

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

melvin-bot[bot] commented 1 day ago

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

cretadn22 commented 1 day ago

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

https://github.com/Expensify/App/blob/be4d7f50949d52aa9f2cb9a8ac495ef19473edde/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx#L28-L35

nkdengineer commented 1 day ago

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

https://github.com/Expensify/App/blob/be4d7f50949d52aa9f2cb9a8ac495ef19473edde/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx#L36

What alternative solutions did you explore? (Optional)

Nodebrute commented 1 day ago

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 https://github.com/Expensify/App/blob/be4d7f50949d52aa9f2cb9a8ac495ef19473edde/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx#L36-L57

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 https://github.com/Expensify/App/blob/be4d7f50949d52aa9f2cb9a8ac495ef19473edde/src/pages/WorkspaceSwitcherPage/index.tsx#L186

FitseTLT commented 1 day ago

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 https://github.com/Expensify/App/blob/cd3f30f73a18c29532d6886b4bb3744fd700ab9d/src/pages/WorkspaceSwitcherPage/index.tsx#L149 https://github.com/Expensify/App/blob/cd3f30f73a18c29532d6886b4bb3744fd700ab9d/src/pages/WorkspaceSwitcherPage/index.tsx#L186

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)

melvin-bot[bot] commented 1 day 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.

twilight2294 commented 1 day ago

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 commented 1 day ago

@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 commented 1 day ago

@JmillsExpensify, isn't this a dupe of https://github.com/Expensify/App/issues/52030?

suneox commented 1 day ago

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

melvin-bot[bot] commented 1 day ago

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

cretadn22 commented 1 day ago

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

suneox commented 20 hours ago

@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 commented 18 hours ago

@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 commented 15 hours ago

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 commented 15 hours ago

@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 commented 15 hours ago

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.