-
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
Directly create a workspace and add the Edit Workspace page #4310
Conversation
…m:Expensify/App into jasper-editWorkspacePage
@tylerkaraszewski Gonna unrequest you for review and request @mountiny to simplify review. He was auto-assigned to review https://github.com/Expensify/Web-Expensify/pull/31579, which this depends on. |
@mountiny This and https://github.com/Expensify/Web-Expensify/pull/31579 should be good for review, thanks! |
@MitchExpensify @jasperhuangg @HorusGoul I guess there might be some merge conflicts coming between this and this PR: #4298 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and code looks good to me, however, there is a lot of stuff which is done in the PR I linked above (which I coincidently review as well 😅 ). I believe you should negotiate, who will merge first, or even merge these two branches to one PR.
I believe based on the nature of the PR by @HorusGoul, which has more changes in terms of design, that one should be merged first. Then just update that code with these functional changes (skipping adding name).
What do you think? 👀 Also we do not need to rush as we should wait for https://github.com/Expensify/Web-Expensify/pull/31579 to be deployed to production.
Ah it looks like I might have done some duplicate work 😅 The issue made it seem like I was supposed to implement everything, but I guess I was only supposed to make it so that clicking "New Workspace" would directly create the workspace. I think it's alright if we merge the other PR first, since it has been back and forth with design. I'll revert the changes in this one that would otherwise conflict. |
@jasperhuangg Thanks! Do you reckon we should still wait or good to merge? Probably good to wait, we should make sure the Web-Expensify changes are deployed before merging this to main as it would be broken for anyone who has local environment pointed to production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works!
@mountiny Yep! Definitely should wait for the Web-E changes to go out on prod before we merge this. Otherwise we'll be creating workspaces without any names 😅 |
@jasperhuangg I see no problem there 😂 |
Good catch on the overlap and finding the best path forward! @jasperhuangg @vitHoracek |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging in version: 1.0.82-8🚀
|
🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀
|
Details
See the issue for more context
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/171960
Tests/QA
You'll need a few accounts with different emails, namely one with a public domain (e.g. test@gmail.com) and two with private domains, one where we have a company name registered (e.g. test@expensify.com), and one where we don't (e.g. test@alksdjflasdf.com).
For both of these accounts:
Tested On
Screenshots
N/A (no UI changes)