Added Workspace Menu #5921

mananjadhav · 2021-10-17T19:04:53Z

cc - @chiragsalian

Details

Adds a generic popup menu to Header with options
Uses this popup menu to add "New workspace" and "Delete Workspace" options

Fixed Issues

$ #5185

Tests

Tested the popup for all platforms - including hiding close button on desktop
Checked the options for New Workspace
Changed by updating language and translations
Pending: Delete Workspace Testing

QA Steps

Go to the right sidebar
Click on any Workspace item
You should see three dots (more options) menu, hover over it
You should see two options - New Workspace and Delete Workspace
Test both the actions are working fine
Try the same steps for mobile, except that the popup should be docked at the bottom

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…ased on dimen

…rkspace-popup-menu

mananjadhav · 2021-10-17T19:10:14Z

Hi @chiragsalian,
I've got the whole PR ready except for the delete API call.

One major question is, I was trying to get the latest pull and it turns out I am not able to access this component. Did we change the flow?

This is the sidebar I see on click. Does the component have to be moved to this one?

mananjadhav · 2021-10-21T03:23:03Z

@chiragsalian Were you able to check the questions in the previous comment ?

chiragsalian · 2021-10-26T22:06:48Z

Hi Manan, sorry for the delay. I must have missed your messages and then i was OOO for a little bit. Anyway feel free to message me on newDot whenever needed since i usually see the messages there faster.

So firstly, while asking a question maybe be more specific on what component you are discussing about. Makes it a little easier to figure out the flow. Okay so let me try to answer now,

I've got the whole PR ready except for the delete API call.

I believe the delete API methods are unchanged so it should work the same as before. Let me know if there is any trouble here.

I was trying to get the latest pull and it turns out I am not able to access this component. Did we change the flow?

So for N6, yeah we changed quite a bit with the front end. Especially during the last week of N6. Number of components did get modified. As for your component, i believe you are talking about this,

If so yes that was unfortunately removed pretty quickly during the last week of N6. Hmm i guess we gotta ask design team if we still want to stick with 3 dots or just show it as another option. I'll ask them now in the issue.

chiragsalian · 2021-10-26T22:17:56Z

I just did a quick post in the issue. I imagine that most likely you've to add the 3 dots next to the close button on the new workspace (which is a slider).

But i've asked design to confirm.

mananjadhav · 2021-10-27T11:07:03Z

Thanks @chiragsalian for the response. I'll do an update on the code and hopefully nothing major in terms of changes.

I believe the delete API methods are unchanged so it should work the same as before. Let me know if there is any trouble here.

I couldn't find this in the current code. If it was added with the new n6-update, I'll have to check. Afaik, it isn't added. Hence, can you help me point to the code where delete is implemented or the API so that I can add the actions on the frontend.

chiragsalian · 2021-10-27T22:59:19Z

Ah yes good question. The endpoint for deleting the workspace is not implemented in App.

But creating it should be easy since we have the endpoint. The API endpoint is called "Policy_Delete" and the argument it needs are authToken and policyID (the policyID to delete). The policyID is seen in the URL whenever you click the workspace.

Adding the delete method should be very similar to how we call Policy_Create like here. Let me know if that helps or if you have any questions around it 👍

…rkspace-popup-menu # Conflicts: # src/components/HeaderWithCloseButton.js # src/languages/en.js # src/languages/es.js # src/pages/workspace/WorkspaceInitialPage.js

mananjadhav · 2021-11-02T20:30:08Z

@chiragsalian I've made updates to the PR and currently testing. Can you help with one last thing?

return Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${response.policyID}`, null);

I am using the above code to unset the value for the policy. How do I delete a key from Onyx?

chiragsalian · 2021-11-05T00:51:03Z

So there is no way to delete a key from Onyx atm. We usually just set it to empty string or null just as you have.
So your code looks good. The only thing i would say is that i don't think request returns a policyID so that code of yours should most likely be,

return Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, null);

Once you update that just make sure to test that setting it to null doesn't break anything else 🙂

…rkspace-popup-menu # Conflicts: # src/components/HeaderWithCloseButton.js # src/libs/actions/Policy.js # src/pages/workspace/WorkspaceInitialPage.js

chiragsalian · 2021-11-16T20:14:02Z

Sorry for the delay. Last week i was really busy with other tasks and then i was on vacation for a few days. Now I'm back 100%. Looking into this. Hmm, the E2E test failure looks odd. As a first step, whenever there is an unusual GH Action test failure first re-run the test, and then update branch with main branch (you can do just the latter if you want since it will trigger travis test). Anyway i've re-run the job. Let's see if it fails at the same spot. I'll review the code in the meantime.

Edit: Looks like rerunning GH actions for E2E tests didn't help. Can you try merging with main and see if that helps.

chiragsalian

Tested the PR. I'm facing a few issues,

The moment a new workspace is created the URL show the reportID i.e., http://localhost:8080/r/255 for me instead of the workspace policyID url i.e., http://localhost:8080/workspace/52E30B06BC2CE462 (the only way to get the correct URL is by closing the workspace menu and opening it again which is weird)
Creating a new workspace ends up in a loop of tons of same network requests. Let me know if you notice the same as well.

chiragsalian · 2021-11-16T20:53:55Z

src/components/ThreeDotsMenu/ThreeDotsMenuItemPropTypes.js

@@ -0,0 +1,11 @@
+import PropTypes from 'prop-types';
+
+


delete extra line break

chiragsalian · 2021-11-16T20:54:03Z

src/components/ThreeDotsMenu/ThreeDotsMenuItemPropTypes.js

+    onPress: PropTypes.func,
+}));
+
+


delete extra line break

chiragsalian · 2021-11-16T20:55:38Z

src/components/ThreeDotsMenu/index.js

+    onIconPress: () => {},
+};
+
+


a line break should just be 1 line break and not 2. Remove the extra line break.

chiragsalian · 2021-11-16T20:59:52Z

src/pages/workspace/WorkspaceInitialPage.js

-                            </Pressable>
-
-                            {policy.name && (
+


remove extra line break.

chiragsalian · 2021-11-16T21:01:34Z

src/libs/actions/Policy.js

+ * @param {Boolean} shouldAutomaticallyReroute
+ * @returns {Promise}
+ */
+function deletePolicy(policyID = '', shouldGrowl = true, shouldAutomaticallyReroute = true) {


Why is policyID optional? It shouldn't be since the backend needs a policyID to know what to delete.

I don't see shouldGrowl and shouldAutomaticallyReroute passed when we call this method. If that's the case let's not preoptimize here and remove these two params from the method call and just do the growl and reroute in the method. In future if there is a scenario to not show growl or reroute then we'll update as needed.

…rkspace-popup-menu # Conflicts: # src/pages/workspace/WorkspaceInitialPage.js # src/styles/styles.js

mananjadhav · 2021-11-20T22:15:19Z

@chiragsalian: PR Updated with the changes. About the two issues:

It seems to be happening due to Navigation.dismissModal() in Policy.js:navigateToPolicy method. If I remove the dismissModal call, it works fine. I am not sure why it was added. Can you confirm?
I had this issue when I raised the first draft, but not later when I submitted with screenshots. Tried creating multiple workspaces and it didn't end into a loop. Can you take the latest pull and check?

mananjadhav · 2021-11-26T22:03:01Z

@chiragsalian Any updates for me on this one? As I've mentioned earlier I'll resolve conflicts once all review is complete or when there are changes. I keep getting conflicts because I've updated the function component to class component.

chiragsalian · 2021-11-30T00:24:58Z

Sorry yes, I was out for Thanksgiving week. Checking this now.

chiragsalian · 2021-11-30T00:49:26Z

It seems to be happening due to Navigation.dismissModal() in Policy.js:navigateToPolicy method. If I remove the dismissModal call, it works fine. I am not sure why it was added. Can you confirm?

My guess is because earlier we use to show a large full screen modal and when we clicked new workspace there we wanted the modal to close. But we don't show that full-screen modal anymore so i'm pretty sure you are safe to remove it. Its usages are also only in two spots so it should be easy enough to confirm if everything runs fine.

I had this issue when I raised the first draft, but not later when I submitted with screenshots. Tried creating multiple workspaces and it didn't end into a loop. Can you take the latest pull and check?

Cool on investigation it looks like its not because of your PR and the issue lies elsewhere.

chiragsalian

Changes LGTM. Feel free to address the conflicts and remove Navigation.dismissModal (test it a bit too)
and then i can merge your PR 👍

…rkspace-popup-menu # Conflicts: # src/pages/workspace/WorkspaceInitialPage.js # src/styles/styles.js

mananjadhav · 2021-11-30T12:09:05Z

Sorry yes, I was out for Thanksgiving week. Checking this now.

@chiragsalian No problem. Thanks for the reviewing, PR updated.

chiragsalian

LGTM and works well. Thank you for the changes.

OSBotify · 2021-11-30T19:12:51Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-12-07T00:21:41Z

🚀 Deployed to staging by @chiragsalian in version: 1.1.17-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-12-08T20:13:30Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron · 2021-12-16T00:20:02Z

My guess is because earlier we use to show a large full screen modal and when we clicked new workspace there we wanted the modal to close. But we don't show that full-screen modal anymore so i'm pretty sure you are safe to remove it. Its usages are also only in two spots so it should be easy enough to confirm if everything runs fine.

It was actually because when creating a new workspace via the LogInWithShortLivedTokenPage we must dismiss the modal that shows a loading spinner here:

App/src/pages/LogInWithShortLivedTokenPage.js

Line 92 in 6433741

return <FullScreenLoadingIndicator />;

And the removal of the call to dismissModal() broke this is not obvious ways e.g.

https://github.com/Expensify/Expensify/issues/188048

This was probably hard (maybe impossible) to see because the line from LoginWithShortLivedTokenPage to policy creation (and which modal needs to be dismissed) isn't super clear.

cc @roryabraham @ctkochan22

mananjadhav · 2021-12-16T02:48:40Z

@marcaaron Then should I add a param to the function and add the Navigation.dismissModal back.

marcaaron · 2021-12-16T02:57:50Z

No worries @mananjadhav we'll take care of it.

mananjadhav · 2021-12-16T03:16:38Z

Thanks, @marcaaron. Do let me know if any help is required.

mananjadhav added 8 commits September 16, 2021 02:08

fix(workspace-popup-menu): Added more options and hide close button b…

4d20e55

…ased on dimen

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wo…

21e630e

…rkspace-popup-menu

fix(wkspace-popup-menu): Added icons and triggers

713e7a0

feat(wkspace-popup-menu): Basic version of the popup menu added

a39816d

feat(wkspace-popup-menu): Moved with menu structure and items

82ea2ca

feat(wkspace-popup-menu): Fixed menu items from header

e95d231

fix(workspace-popup-menu): Added create call

19ad6d0

fix(wkspace-popup-menu): Changed function call

f4dc5c3

feat(wkspace-popup-menu): Made offset to props

409fc3d

mananjadhav added 8 commits November 2, 2021 21:49

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wo…

67812fa

…rkspace-popup-menu # Conflicts: # src/components/HeaderWithCloseButton.js # src/languages/en.js # src/languages/es.js # src/pages/workspace/WorkspaceInitialPage.js

fix(wkspace-popup-menu): Discarded extra Header component

06a39c9

fix(wkspace-popup-menu): Removed hook and replaced with getPathFromState

ffa5b9c

fix(wkspace-popup-menu): Changed function component to class component

90d0453

fix(wkspace-popup-menu): Added Confirm delete handling

0fb02ff

fix(wkspace-popup-menu): Fixed keys for Delete modal

6f36f93

fix(wkspace-popup-menu): Added delete policy API

03c0676

fix(wkspace-popup-menu): Added comments to the policy

4b88a10

mananjadhav added 5 commits November 8, 2021 13:37

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wo…

ddced90

…rkspace-popup-menu # Conflicts: # src/components/HeaderWithCloseButton.js # src/libs/actions/Policy.js # src/pages/workspace/WorkspaceInitialPage.js

fix(wkspace-popup-menu): Converted from function to class component

6a3b23d

fix(wkspace-popup-menu): Fixed create and delete functions

9dbfbf1

fix(wkspace-popup-menu): Added growl on delete success

c38131f

fix(wkspace-popup-menu): Changed PopoverMenu component

ea91fbb

chiragsalian requested changes Nov 16, 2021

View reviewed changes

mananjadhav added 3 commits November 21, 2021 02:49

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wo…

20ea9d2

…rkspace-popup-menu # Conflicts: # src/pages/workspace/WorkspaceInitialPage.js # src/styles/styles.js

fix(wkspace-popup-menu): Fixed lint issues and typos

d50cdd9

fix(wkspace-popup-menu): Removed extra params from deletePolicy

89d35b5

mananjadhav requested a review from chiragsalian November 29, 2021 12:13

chiragsalian previously approved these changes Nov 30, 2021

View reviewed changes

mananjadhav added 2 commits November 30, 2021 17:04

Merge branch 'main' of https://github.com/mananjadhav/App into fix/wo…

f9a0bcc

…rkspace-popup-menu # Conflicts: # src/pages/workspace/WorkspaceInitialPage.js # src/styles/styles.js

fix(wkspace-popup-menu): Lint fixes

5d37753

mananjadhav dismissed chiragsalian’s stale review via 5d37753 November 30, 2021 12:05

fix(wkspace-popup-menu): Removed Navigation.dismissModal

39d8a14

chiragsalian approved these changes Nov 30, 2021

View reviewed changes

chiragsalian merged commit ff3625f into Expensify:main Nov 30, 2021

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

botify mentioned this pull request Dec 8, 2021

[HOLD for payment 2021-12-27] [HOLD for payment 2021-12-15] Add "Workspace" to the Workspace menu on the left pane header on Desktop/Web and button to delete/add new workspaces #5185

Closed

mananjadhav mentioned this pull request Dec 14, 2021

Workspace - Previous workspace details shown when navigating away after creation - Reported by: @Sahil Khan #6716

Closed

marcaaron mentioned this pull request Dec 17, 2021

Remove early return for workspace new #6784

Merged

5 tasks

mollfpr mentioned this pull request Mar 27, 2022

[HOLD for payment 2022-04-13] [$750] When clicking on Room Settings after workspace deleted, error displayed - reported by @Santhosh-Sellavel #8033

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Workspace Menu #5921

Added Workspace Menu #5921

mananjadhav commented Oct 17, 2021 •

edited

Loading

mananjadhav commented Oct 17, 2021

mananjadhav commented Oct 21, 2021

chiragsalian commented Oct 26, 2021

chiragsalian commented Oct 26, 2021

mananjadhav commented Oct 27, 2021

chiragsalian commented Oct 27, 2021

mananjadhav commented Nov 2, 2021

chiragsalian commented Nov 5, 2021

chiragsalian commented Nov 16, 2021 •

edited

Loading

chiragsalian left a comment •

edited

Loading

chiragsalian Nov 16, 2021

chiragsalian Nov 16, 2021

chiragsalian Nov 16, 2021

chiragsalian Nov 16, 2021

chiragsalian Nov 16, 2021

chiragsalian Nov 16, 2021

mananjadhav commented Nov 20, 2021

mananjadhav commented Nov 26, 2021

chiragsalian commented Nov 30, 2021

chiragsalian commented Nov 30, 2021

chiragsalian left a comment •

edited

Loading

mananjadhav commented Nov 30, 2021

chiragsalian left a comment

OSBotify commented Nov 30, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

marcaaron commented Dec 16, 2021

mananjadhav commented Dec 16, 2021

marcaaron commented Dec 16, 2021

mananjadhav commented Dec 16, 2021

Added Workspace Menu #5921

Added Workspace Menu #5921

Conversation

mananjadhav commented Oct 17, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mananjadhav commented Oct 17, 2021

mananjadhav commented Oct 21, 2021

chiragsalian commented Oct 26, 2021

chiragsalian commented Oct 26, 2021

mananjadhav commented Oct 27, 2021

chiragsalian commented Oct 27, 2021

mananjadhav commented Nov 2, 2021

chiragsalian commented Nov 5, 2021

chiragsalian commented Nov 16, 2021 • edited Loading

chiragsalian left a comment • edited Loading

Choose a reason for hiding this comment

chiragsalian Nov 16, 2021

Choose a reason for hiding this comment

chiragsalian Nov 16, 2021

Choose a reason for hiding this comment

chiragsalian Nov 16, 2021

Choose a reason for hiding this comment

chiragsalian Nov 16, 2021

Choose a reason for hiding this comment

chiragsalian Nov 16, 2021

Choose a reason for hiding this comment

chiragsalian Nov 16, 2021

Choose a reason for hiding this comment

mananjadhav commented Nov 20, 2021

mananjadhav commented Nov 26, 2021

chiragsalian commented Nov 30, 2021

chiragsalian commented Nov 30, 2021

chiragsalian left a comment • edited Loading

Choose a reason for hiding this comment

mananjadhav commented Nov 30, 2021

chiragsalian left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 30, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

marcaaron commented Dec 16, 2021

mananjadhav commented Dec 16, 2021

marcaaron commented Dec 16, 2021

mananjadhav commented Dec 16, 2021

mananjadhav commented Oct 17, 2021 •

edited

Loading

chiragsalian commented Nov 16, 2021 •

edited

Loading

chiragsalian left a comment •

edited

Loading

chiragsalian left a comment •

edited

Loading