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

[$250] Web - Settings - Save button looks clickable after changing and saving preferred pronoun(s) #8364

Closed
kbecciv opened this issue Mar 30, 2022 · 48 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement.

Comments

@kbecciv
Copy link

kbecciv commented Mar 30, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to http://staging.new.expensify.com
  2. Log in to any account that has the first and last name fields in the profile filled, and click the settings icon
  3. In the "Settings" section on the right, click "Profile"
  4. Click the "Preferred Pronouns" dropdown, select any option and click "Save"
  5. After the confirmation the profile has been updated appears, hover on the "Save" button and click it

Expected Result:

I expected the Save button to have a faded green "greyed out" appearance.

Actual Result:

Button looks clickable due to its saturated green color.

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.1.46.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): [email protected]/1q2w3e$R

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:
Updated vid on 2022-11-02

Recording.1502.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause (Exploratory)

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Mar 30, 2022

Triggered auto assignment to @yuwenmemon (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@yuwenmemon yuwenmemon added Weekly KSv2 Improvement Item broken or needs improvement. and removed Daily KSv2 labels Mar 30, 2022
@melvin-bot melvin-bot bot added the Overdue label Apr 7, 2022
@yuwenmemon
Copy link
Contributor

Sorry, not update here but will take a look soon.

@melvin-bot melvin-bot bot removed the Overdue label Apr 7, 2022
@melvin-bot melvin-bot bot added the Overdue label Apr 18, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@yuwenmemon
Copy link
Contributor

Can't reproduce locally either. Closing!

@melvin-bot melvin-bot bot removed the Overdue label Apr 19, 2022
@mallenexpensify
Copy link
Contributor

Having a similar issue with the save button still being green after updates have been made. Skip to the 20-second mark of the vid below.

2022-10-24_07-56-07.mp4

@kbecciv , can you see if you're able to reproduce.

@kbecciv
Copy link
Author

kbecciv commented Oct 24, 2022

@mallenexpensify Checking, update you shortly.

@kbecciv
Copy link
Author

kbecciv commented Oct 24, 2022

I'm able to reproduce it with build 1.2.18.5

Recording.1502.mp4

@mallenexpensify
Copy link
Contributor

Thanks @kbecciv
@yuwenmemon , I'm assuming this can be an External issue, right?!?!!?

@mallenexpensify mallenexpensify added Daily KSv2 and removed Weekly KSv2 labels Oct 24, 2022
@melvin-bot melvin-bot bot added the Overdue label Oct 27, 2022
@mallenexpensify
Copy link
Contributor

@yuwenmemon , can you confirm this can/can't be External plz?

@mallenexpensify
Copy link
Contributor

Should it be saturated if the user just entered the Profile page and hasn't made any changes yet?

I think my wording isn't/wasn't clear.

  • user just entered the Profile page and hasn't made any changes = greyed out cuz nothing's been changed
  • If a change is made that needs to be saved = saturated
  • Once change has been saved = greyed out.

@mallenexpensify
Copy link
Contributor

Looks like I forgot to create Upwork job earlier, just did and doubled price to $500
https://www.upwork.com/jobs/~0167fcf362c491d826

@eVoloshchak
Copy link
Contributor

  • user just entered the Profile page and hasn't made any changes = greyed out cuz nothing's been changed
  • If a change is made that needs to be saved = saturated
  • Once change has been saved = greyed out.

Makes sense, thanks

@vincentrohde
Copy link

@eVoloshchak @mallenexpensify is this issue still up for grabs?

@mallenexpensify
Copy link
Contributor

@vincentrohde All issues with Help Wanted should be awaiting proposals. Be sure to familiarize yourself with out https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md.

@dev-attiq
Copy link

When I visit the profile page https://staging.new.expensify.com/settings/profile, the save button always looks clickable it doesn't matter if we change anything.
What we can do is, by default, we will set the button state which will change the button color to faded green and the button will not be clickable.
We will keep track of the changes made on the profile page and if there is something that changes on the profile page, we will set the button state, the color of the button will be changed to green and the button will be clickable as well.

@tlerias
Copy link

tlerias commented Nov 5, 2022

Issue #8364 Proposal

Author: @tlerias

Proposal:

The Problem:
Please see the FORMS.md, which states: Submit buttons shall not be disabled or blocked from being pressed in most cases. We will allow the user to submit a form and point them in the right direction if anything needs their attention. With that being said, there are a couple approaches we can take to address this issue:

Option 1: Add a form alert.

We can leave the Submit button "saturated" but display a form error message explaining that the user has not made any changes.

Technical Approach: We can add a validation to check the previous state with the current state to see if any fields have changed. We potentially can use the touchedInput state field here as well. If no fields have changed, then we display an error message just above the Submit button to the user.

Recommended Option 2: Disable the Save Button for all Forms when the user has not made any changes.

We can disable the save button if the user has not made any changes to the form. Note: this will change the behavior wherever the Form component is used, therefore we should change the FORMS.md readme in this case.

Technical Approach: We add an isDisabled attribute to the Form Component and set to true if any inputs have not been touched. This will require us to on submit to reset the state.touchedInputs to be an empty object AFTER validation happens.

I recommend this approach because it will be the cleanest code to add and will be available on all forms.

Option 3: Disable the Save Button for just the Profile form.

We can disable the save button if the user has not made any changes to only the Profile form. Note this will add some extra conditionals to the code base which doesn't really make sense to make it work for just this one form. This is also a problem on other forms (like the Change Password Form).

Technical Approach: This technical approach will be the same for Option 2, but I will have to add in additional checks to see which form the user is on. I do not recommend this approach, though left it here as an option to show it has been thought through.

Thanks for the opportunity to submit a proposal!

@melvin-bot melvin-bot bot added the Overdue label Nov 7, 2022
@juzoace
Copy link

juzoace commented Nov 7, 2022

@mallenexpensify , I sent an email to join the slack group so I can contribute......still yet to get a response

@eVoloshchak
Copy link
Contributor

@tlerias , thank you for the proposal
Based on this comment we should go with options 2 or 3.

2: Disable the Save Button for all Forms when the user has not made any changes.
3: Disable the Save Button for just the Profile form.

@mallenexpensify , is this change needs to be implemented only for the Profile page?

@melvin-bot melvin-bot bot removed the Overdue label Nov 7, 2022
@mallenexpensify
Copy link
Contributor

@sketchydroide what do you think? I feel like #3 is safer, to ensure we don't maybe mess something up but, #3 seems like how it should always work... right?!?!

@mallenexpensify mallenexpensify removed their assignment Nov 9, 2022
@mallenexpensify mallenexpensify added the Bug Something is broken. Auto assigns a BugZero manager. label Nov 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@mallenexpensify
Copy link
Contributor

@trjExpensify Reassigning as I'm heading OOO for a month

@trjExpensify
Copy link
Contributor

👋 I don't think this is a problem as stated:

I expected the Save button to have a faded green "greyed out" appearance.

Here in our Forms.MD we describe the behaviour of the save button, specifically, that it's not disabled or blocked from being pressed.

Submit buttons shall not be disabled or blocked from being pressed in most cases. We will allow the user to submit a form and point them in the right direction if anything needs their attention.

Before the API refactors, you would get a growl onPress for feedback that the changes have been saved. With that initiative the growls went away to facilitate taking an optimistic action and adding the red brick road (RBR) error message in-line if it happens to fail.

Further, we're going to change this page quite considerably soon as part of N7 account settings, so it follows the push-to-page pattern. (CC: @Beamanator, @twisterdotcom, @zanyrenney).

In the meantime, recognising the UX isn't great in the limbo period, I think we take a similar minimal approach to what we discussed for the workspace > general settings page and return the user to the previous page when clicking the Save button.

Thoughts?

@JmillsExpensify
Copy link

In the meantime, recognising the UX isn't great in the limbo period, I think we take a similar minimal approach to what we discussed for the workspace > general settings page and return the user to the previous page when clicking the Save button.

I agree with this. Great point.

@sketchydroide
Copy link
Contributor

In all honesty I think the button should no be visible is there are no changes to save, if there is no action the user can take, then there should be no action visible. So the rule of no button never being disabled would work.
As it is, and the button looking active it breaks Apple rules (not much and I don't think they will say anything, but we never know).
I think if we are making changes to this provess, we can wait for that to be done.

@trjExpensify
Copy link
Contributor

Yeah, this page won't exist soon enough. Do you agree with minimally doing the below, like we will on the workspace > general settings page?

return the user to the previous page when clicking the Save button.

@Beamanator
Copy link
Contributor

FYI there's 5 new pages that are being built (push-to-page style) to replace the Profile page. They are:

  1. display name: Add new Display Name page #12142 (in staging)
  2. timezone pages: New timezone pages #12201 (@cristipaval taking over)
  3. pronouns page: New Pronouns page #12301 (still some outstanding implementation discussions)
  4. contact methods pages -> held on #passwordless discussions
  5. private personal details -> hold on WAQ

The first 3 can probably be done by end of next week, then we can put them all on the current Profile Page proooobably, wait for #passwordless for point 4, and wait for WAQ to be "done" for point 5

@trjExpensify
Copy link
Contributor

Ah nice, so those are in the works. Do you agree with do nothing then and close this issue?

@JmillsExpensify
Copy link

The first 3 can probably be done by end of next week, then we can put them all on the current Profile Page proooobably, wait for #passwordless for point 4, and wait for WAQ to be "done" for point 5

Oh nice, this sounds like a really good plan btw.

@JmillsExpensify
Copy link

pronouns page: #12301 (still some outstanding implementation discussions)

So what's the story on removing the custom option? Are we removing that in this PR? Or is that being separated off to it's own distinct project? I think latter is probably the most logical at this point, though wanted to confirm. cc @zanyrenney (I tried to find Ted Harris via his username but nothing was coming up).

@Beamanator
Copy link
Contributor

@twisterdotcom likes to be sneaky with his GH username 😉

Or is that being separated off to it's own distinct project? I think latter is probably the most logical at this point, though wanted to confirm.

I like this idea, implementing the page w/out the custom pronoun till we figure out exactly what we want to do there - this way we can implement the Pronouns page quite quickly 👍

@trjExpensify
Copy link
Contributor

Circling back to this issue. Do we agree to close it and not make any changes in the interim given that the re-design to pronouns is on its way?

@Beamanator
Copy link
Contributor

Agree 👍 (close b/c we won't have a save button soon)

@trjExpensify
Copy link
Contributor

Perfecto, thank ya'll!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement.
Projects
None yet
Development

No branches or pull requests