Skip to content
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

chg: Workspace Edit Page #5271

Merged
merged 8 commits into from
Oct 25, 2021
Merged

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Sep 15, 2021

Details

  1. Change the Form Input Field.
  2. Added the error message.
  3. Refactor to fix the issue.

Fixed Issues

$ #5194
$ #5783
$ #5777

Tests | QA Steps

  1. Navigate to settings
  2. Click on a workspace
  3. Click on the workspace avatar to edit
  4. Delete the name and save.
  5. You should error below the field.
  6. Now add a new Name and save and save should happen smoothly.

Please test all the three issues attached.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

image

Mobile Web

Screenshot 2021-09-15 23:45:17

iOS

Android

Screenshot 2021-09-15 23:45:17

@parasharrajat parasharrajat requested a review from a team as a code owner September 15, 2021 18:12
@MelvinBot MelvinBot requested review from marcaaron and removed request for a team September 15, 2021 18:12
@marcaaron marcaaron requested review from johnmlee101 and removed request for marcaaron September 15, 2021 18:21
@kadiealexander
Copy link
Contributor

Hi @parasharrajat, I've placed this issue on hold as per this update, as we are prioritising issues related to a feature release scheduled for 9/24. As an apology for the delay, we will add a $100 bonus as a thank you for waiting.

@parasharrajat
Copy link
Member Author

parasharrajat commented Oct 18, 2021

@johnmlee101

Workspace Settings Page updates in N6 and thus the issue #5194 no more exists.

But It covers other existing bugs which was not fixed.

  • Pressing submits when the image is still uploading.
  • Clear the Name and pressing the Save, submits the form even if there is an error.

Secondly, I am fixing another issue in this PR as I don't want to create another PR for the small issue #5783.

@parasharrajat
Copy link
Member Author

There is another issue that I will take care of in this one. #5777.

But I want to know the best solution possible for that.

  1. Should we block the navigation when the image is uploading and show an alert?
  2. Gracefully hide handle the Policy. i.e. Set the isAvatarUploading: false and Block the state update.

@johnmlee101
Copy link
Contributor

Should we block the navigation when the image is uploading and show an alert?

I think just disabling the inputs until finished is fine.

Gracefully hide handle the Policy. i.e. Set the isAvatarUploading: false and Block the state update.

and yeah blocking the state update makes the most sense.

@parasharrajat
Copy link
Member Author

@johnmlee101 This is ready for review.

@parasharrajat
Copy link
Member Author

Gentle bump @johnmlee101 for review.

Copy link
Contributor

@johnmlee101 johnmlee101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, but if you think it should be a NAB lmk

src/pages/workspace/WorkspaceSettingsPage.js Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

@johnmlee101 Answered your question. Let's merge otherwise.

@johnmlee101 johnmlee101 merged commit 4918251 into Expensify:main Oct 25, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @johnmlee101 in version: 1.1.8-23 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants