Add a modal screen for creating a new workspace #3547

HorusGoul · 2021-06-11T14:29:35Z

Details

Adds a new modal screen for creating workspaces (UI only this is not connected to the backend yet). Only accessible through the web with the path /workspace/new for now.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/166035

Tests

Open E.cash and log in.
Change the URL to http://localhost:8080/workspace/new
Make sure everything looks like it should.

QA Steps

Open E.cash and log in.
Change the URL to https://staging.expensify.cash/workspace/new
Make sure everything looks like it should.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2021-06-11T14:29:56Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

HorusGoul · 2021-06-11T14:30:07Z

I have read the CLA Document and I hereby sign the CLA

…w-workspace

marcaaron

Looks really great, I don't have too many critiques (only NABs). Nice job! :)

marcaaron · 2021-06-11T18:09:37Z

src/pages/workspace/NewWorkspacePage.js

+                                />
+                            </>
+                        )}
+                    </AttachmentPicker>


NAB, this might be a case where we can convert this pattern into a slightly more complex component. But we can perhaps wait until it is used a third time? Just something I thought about since we have mostly the same UI pattern used in the ProfilePage.js

marcaaron · 2021-06-11T18:10:38Z

src/pages/workspace/NewWorkspacePage.js

+}
+
+NewWorkspacePage.propTypes = propTypes;
+NewWorkspacePage.displayName = 'NewWorkspacePage';


NAB, class components do not require this displayName

marcaaron · 2021-06-11T18:12:28Z

src/pages/workspace/NewWorkspacePage.js

+     * @param {Function} openPicker
+     * @returns {Array}
+     */
+    createMenuItems(openPicker) {


NAB, these options really only need to be updated if the translation theme changes, but I don't think it's worth optimizing for so this function works for me.

I left the function because we'll need it when we connect this with the backend, and we need to decide whether to show the Delete Image item or not based on the state.

marcaaron · 2021-06-11T18:15:47Z

src/pages/workspace/NewWorkspacePage.js

+    }
+
+    render() {
+        const {translate} = this.props;


NAB, IIRC we have some kind of specific advice on when to destructure something in the style guide. but generally prefer explicit use of this.props when there is a mix of props and state to avoid confusion about which things come from where. That said, t's quite easy to see what's happening here so I won't block.

marcaaron · 2021-06-11T18:17:44Z

Oh yes just remembered something. Technically this should be behind a beta... so we'll need to block access for now.

Here's a strategy I used to achieve this that you can copy.

https://github.com/Expensify/Expensify.cash/blob/19ad75ea6b6905ce31f37b5e8251d98e23add5a1/src/pages/BusinessBankAccount/NewPage.js#L72-L76

mountiny

Great update! Works well in my browser. i will not merge it now as Marc has mentioned the beta access.

HorusGoul · 2021-06-14T09:42:28Z

@marcaaron That works! However, I found this error caused by the Navigation.dismissModal(); call:

It's definitely related to that call being a side effect, and side effects inside the render function are illegal. We can solve that error by creating a utility component to dismiss modals in a declarative way, like this:

 if (!Permissions.canUseFreePlan(this.props.betas)) {
       console.debug('Not showing new bank account page because user is not on free plan beta');
       return <Navigation.DismissModal />;  
 }

I went ahead and added the change to this PR in a separate commit. Let me know what you think.

mountiny · 2021-06-14T12:23:14Z

src/pages/workspace/NewWorkspacePage.js

+        if (!Permissions.canUseFreePlan(this.props.betas)) {
+            console.debug('Not showing new workspace page because user is not on free plan beta');
+            return <Navigation.DismissModal />;
+        }


Good call! Let's then not forget to refactor the rest of the app to use this.

OSBotify · 2021-06-14T16:45:09Z

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

marcaaron · 2021-06-14T17:10:44Z

src/pages/workspace/NewWorkspacePage.js

+                onSelected: () => {
+                    openPicker({
+                        onPicked: () => {
+                            // TODO: connect with setWorkspaceAvatar function


@HorusGoul are you up for creating a follow up issue to track the changes we need to make to "complete" this?

OSBotify · 2021-06-15T17:14:31Z

🚀 Deployed to staging in version: 1.0.68-5🚀

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

isagoico · 2021-06-15T20:47:17Z

@HorusGoul I'm unable to access the link https://staging.expensify.cash/workspace/new (even with a expensifail account) is there any permissions needed to be able to see the page?

HorusGoul · 2021-06-16T07:45:33Z

I can't access the page in staging either. @marcaaron, do we have enabled the free plan beta in staging/prod for our accounts?

marcaaron · 2021-06-16T16:37:03Z

Hmm I don't think there is any difference between betas on staging vs. production. Something else might be happening here. I checked my localStorage in staging.expensify.cash and did not see the freePlan beta there

However, pn both production and staging I was able to see that the freePlan beta is available on my expensifail.com account.

Here's where that list is pulled from
https://github.com/Expensify/Web-Expensify/blob/03d5b177058f6c1343fec5eea9233a8411c8ca51/lib/BetaManager.php#L186-L204

OSBotify · 2021-06-25T18:32:45Z

🚀 Deployed to production in version: 1.0.73-3🚀

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

HorusGoul added 3 commits June 9, 2021 16:57

Add NewWorkspace page

379b104

Add the layout for the new workspace modal

d6920b9

Add AttachmentPicker and use the workspace default avatar

4ad470f

HorusGoul self-assigned this Jun 11, 2021

HorusGoul added 2 commits June 11, 2021 16:36

Merge remote-tracking branch 'origin/main' into horus-modal-screen-ne…

1fe8daa

…w-workspace

Add request a call switch

b314b0c

HorusGoul changed the title ~~[WIP] Add a modal screen for creating a new workspace~~ Add a modal screen for creating a new workspace Jun 11, 2021

HorusGoul marked this pull request as ready for review June 11, 2021 16:30

HorusGoul requested a review from a team as a code owner June 11, 2021 16:30

MelvinBot requested review from mountiny and removed request for a team June 11, 2021 16:30

HorusGoul requested a review from marcaaron June 11, 2021 16:30

marcaaron previously approved these changes Jun 11, 2021

View reviewed changes

mountiny previously approved these changes Jun 12, 2021

View reviewed changes

HorusGoul added 2 commits June 14, 2021 11:18

Apply PR suggestions

7a21fbc

Add Navigation.DismissModal component

5c1003f

HorusGoul dismissed stale reviews from mountiny and marcaaron via 5c1003f June 14, 2021 09:37

HorusGoul added 2 commits June 14, 2021 11:43

Fix example in Navigation.js

eea9c9d

Use this.props.translate instead of destructuring

9872661

HorusGoul requested review from marcaaron and mountiny June 14, 2021 09:54

mountiny approved these changes Jun 14, 2021

View reviewed changes

marcaaron approved these changes Jun 14, 2021

View reviewed changes

marcaaron merged commit bdf2144 into main Jun 14, 2021

marcaaron deleted the horus-modal-screen-new-workspace branch June 14, 2021 16:44

marcaaron reviewed Jun 14, 2021

View reviewed changes

OSBotify mentioned this pull request Jun 15, 2021

Deploy Checklist: Expensify.cash 2021-06-15 #3593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a modal screen for creating a new workspace #3547

Add a modal screen for creating a new workspace #3547

HorusGoul commented Jun 11, 2021 •

edited

Loading

github-actions bot commented Jun 11, 2021 •

edited

Loading

HorusGoul commented Jun 11, 2021

marcaaron left a comment

marcaaron Jun 11, 2021

marcaaron Jun 11, 2021

HorusGoul Jun 14, 2021

marcaaron Jun 11, 2021

HorusGoul Jun 14, 2021

marcaaron Jun 11, 2021

HorusGoul Jun 14, 2021

marcaaron commented Jun 11, 2021

mountiny left a comment

HorusGoul commented Jun 14, 2021 •

edited

Loading

mountiny Jun 14, 2021

OSBotify commented Jun 14, 2021

marcaaron Jun 14, 2021

HorusGoul Jun 15, 2021

OSBotify commented Jun 15, 2021

isagoico commented Jun 15, 2021

HorusGoul commented Jun 16, 2021

marcaaron commented Jun 16, 2021

OSBotify commented Jun 25, 2021

Add a modal screen for creating a new workspace #3547

Add a modal screen for creating a new workspace #3547

Conversation

HorusGoul commented Jun 11, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Jun 11, 2021 • edited Loading

HorusGoul commented Jun 11, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Jun 11, 2021

mountiny left a comment

Choose a reason for hiding this comment

HorusGoul commented Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

OSBotify commented Jun 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jun 15, 2021

isagoico commented Jun 15, 2021

HorusGoul commented Jun 16, 2021

marcaaron commented Jun 16, 2021

OSBotify commented Jun 25, 2021

HorusGoul commented Jun 11, 2021 •

edited

Loading

github-actions bot commented Jun 11, 2021 •

edited

Loading

HorusGoul commented Jun 14, 2021 •

edited

Loading