-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[$250] Remove plus icon in the workspace switcher when you already have at least one workspace #52409
Comments
Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox ( |
Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new. |
ProposalPlease 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 App/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx Lines 28 to 35 in be4d7f5
|
ProposalPlease 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
What alternative solutions did you explore? (Optional) |
Edited by proposal-police: This proposal was edited at 2024-11-12 16:28:26 UTC. ProposalPlease 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 App/src/pages/WorkspaceSwitcherPage/WorkspacesSectionHeader.tsx Lines 36 to 57 in be4d7f5
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 App/src/pages/WorkspaceSwitcherPage/index.tsx Line 186 in be4d7f5
|
ProposalPlease 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 App/src/pages/WorkspaceSwitcherPage/index.tsx Line 149 in cd3f30f
App/src/pages/WorkspaceSwitcherPage/index.tsx Line 186 in cd3f30f
What changes do you think we should make in order to solve the problem?We should show it when the workspaces list is zero
What alternative solutions did you explore? (Optional) |
|
ProposalPlease 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 --- 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) |
@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! |
@JmillsExpensify, isn't this a dupe of #52030? |
Since WorkspacesSectionHeader is only used on WorkspaceSwitcherPage and now it only contains a text header after removing the plus add icon We can go with @cretadn22 alternative proposal 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@cretadn22 Your alternative solution is missing container style |
@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 |
Based on contributingGuides item 6 in Propose a solution for the job about
I'd like to update selected proposal to @cretadn22 alternative proposal. Thanks for all proposal cc: @marcochavezf |
@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 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 When we implement @cretadn22 solution, the result we get is: 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 |
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. |
I agree with @Nodebrute; given the nature of the problem and the proposed solutions, it's difficult to determine which approach is objectively better. That said, I appreciate your effort in defending your solution @twilight2294. I will respect @suneox's decision on this one, but I’m confident you'll have better luck with the next issue 🚀 |
Thanks, indeed hard luck but looking to contribute further |
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:
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.
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.
Issue Owner
Current Issue Owner: @suneoxThe text was updated successfully, but these errors were encountered: