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

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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): totallyaboveboard+alpha@gmail.com/1q2w3e$R

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

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

https://user-images.githubusercontent.com/93399543/197567558-13e425c5-c66b-4ff0-b231-6a806df29916.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause (Exploratory)

Slack conversation:

View all open jobs on GitHub

[melvin-bot[bot]]

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

[yuwenmemon]

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

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[yuwenmemon]

Can't reproduce locally either. Closing!

[mallenexpensify]

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.

https://user-images.githubusercontent.com/22508304/197558783-a08956f1-e8f7-4688-8f97-34441784598c.mp4

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

[kbecciv]

@mallenexpensify Checking, update you shortly.

[kbecciv]

I'm able to reproduce it with build 1.2.18.5

https://user-images.githubusercontent.com/93399543/197567558-13e425c5-c66b-4ff0-b231-6a806df29916.mp4

[mallenexpensify]

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

[mallenexpensify]

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

[melvin-bot[bot]]

Current assignee @mallenexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[melvin-bot[bot]]

Triggered auto assignment to @sketchydroide (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[furkanuzundev]

Proposal

The problem is coming from <FormAlertWithSubmitButton /> component. Component has a prop named:

/** Whether the button is disabled */
    isDisabled: PropTypes.bool,

But there is nowhere to use it into <Form />

Workaround

1. I created a field which named buttonDisabledComparedFields.This fields are includes the same name with component inputID and it's working dynamically.

diff --git a/src/pages/settings/Profile/ProfilePage.js b/src/pages/settings/Profile/ProfilePage.js
index e6ee4c6fe..d4cf4a1ed 100755
--- a/src/pages/settings/Profile/ProfilePage.js
+++ b/src/pages/settings/Profile/ProfilePage.js
@@ -236,6 +236,16 @@ class ProfilePage extends Component {
                     onSubmit={this.updatePersonalDetails}
                     submitButtonText={this.props.translate('common.save')}
                     enabledWhenOffline
+                    buttonDisabledComparedFields={
+                        {
+                            firstName: lodashGet(currentUserDetails, 'firstName', null),
+                            lastName: lodashGet(currentUserDetails, 'lastName', null),
+                            timezone: lodashGet(currentUserDetails, 'timezone.selected', null),
+                            isAutomaticTimezone: lodashGet(currentUserDetails, 'timezone.automatic', null),
+                            pronouns: pronounsPickerValue,
+                            ...(this.state.hasSelfSelectedPronouns && {selfSelectedPronoun: lodashGet(currentUserDetails, 'pronouns', null)}),
+                        }
+                    }
                 >
                     <OfflineWithFeedback
                         pendingAction={lodashGet(this.props.currentUserPersonalDetails, 'pendingFields.avatar', null)}

2. Get this fields from base <Form /> component.

diff --git a/src/components/Form.js b/src/components/Form.js
index b7b5f72ed..5f67a6e06 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -48,6 +48,10 @@ const propTypes = {
     /** Should the button be enabled when offline */
     enabledWhenOffline: PropTypes.bool,

+    /** Check input state with default values for submit button disabled */
+    // eslint-disable-next-line react/require-default-props
+    buttonDisabledComparedFields: PropTypes.objectOf(PropTypes.string),
+
     ...withLocalizePropTypes,
 };

3. And compare to this fields with updated input state with isMatch function from underscore into <FormAlertWithSubmitButton

@@ -232,6 +236,7 @@ class Form extends React.Component {
                             }}
                             containerStyles={[styles.mh0, styles.mt5]}
                             enabledWhenOffline={this.props.enabledWhenOffline}
+                            isDisabled={!_.isEmpty(this.props.buttonDisabledComparedFields) ? _.isMatch(this.state.inputValues, this.props.buttonDisabledComparedFields) : false}
                         />
                         )}
                     </View>

Result

If <Form /> component catch any changes, save button disabled changing immediately.

https://user-images.githubusercontent.com/30953070/198387382-857c86f2-c0f3-49d7-8aa0-4d5996b69836.mov

[furkanuzundev]

anyone here? @mallenexpensify

[eVoloshchak]

anyone here?

Will review the proposal on Monday

[eVoloshchak]

I'm unable to reproduce this issue When building from source (and on staging?.new.expensify.com too), the Save button is always in enabled state. There's also no loading spinner or success message

https://user-images.githubusercontent.com/9059945/199043072-b38c1bc7-d74b-4580-a4a0-9b68ecfe501a.mov

[furkanuzundev]

@eVoloshchak Spinner and success message is related this issue, but they are not related with this subject.So, i just tried to fix save button clickable when changes detect.

If you want to fix all the issues about it like this;

1. If user hasn't change anything, save button disabled.
2. If user change something, save button enabled.
3. If user changed and click save button, display spinner until save is done.
4. When save operation is done, show the response message.

We need to work on them one by one.

[eVoloshchak]

This was working at some point, I think it's better to find what broke it. @furkanuzundev, you are right, this is a separate issue. I'll try to look for the PR that caused this, expect an update here in ~24h

[furkanuzundev]

sure.

[eVoloshchak]

Found this discussion on Slack, which makes the desired behavior less clear

Just noticed, that we should probably update the issue description, video there does not represent current behavior. This one does.

@mallenexpensify Could you please update the issue description with the correct video and the expected result? Should the button be grey when the user just enters the Profule screen? Should a success message be shown after saving the user's profile? (that's how it worked before, so i'm unsure what is the expected result here)

[mallenexpensify]

Good catch @eVoloshchak, question for you and @sketchydroide, should we split this into two issues or keep as one? Reasoning, there are two issues.

1. Once you click into Profile, the save button should be saturated green showing that it should be clicked to save something
2. After a change is made in Profile and the Save button is clicked, it shouldn't stay saturated green.

Not sure if they're directly related and would be fixed by the same PR or not.

[furkanuzundev]

I think the same PR can be worked on, as they both work directly in conjunction with the save function.

[eVoloshchak]

@mallenexpensify

should we split this into two issues or keep as one? Not sure if they're directly related and would be fixed by the same PR or not.

I think they should be kept as one issue, this is a single flow in my opinion.

Once you click into Profile, the save button should be saturated green showing that it should be clicked to save something

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

[furkanuzundev]

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

I agree with it.

[mallenexpensify]

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]

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

[eVoloshchak]

• 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]

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

[mallenexpensify]

@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]

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]

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!

[juzoace]

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

[eVoloshchak]

@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?

[mallenexpensify]

@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?!?!

[melvin-bot[bot]]

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

[mallenexpensify]

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

[trjExpensify]

👋 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]

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]

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]

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]

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

1. display name: https://github.com/Expensify/App/pull/12142 (in staging)
2. timezone pages: https://github.com/Expensify/App/pull/12201 (@cristipaval taking over)
3. pronouns page: https://github.com/Expensify/App/pull/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]

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

[JmillsExpensify]

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]

pronouns page: https://github.com/Expensify/App/pull/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]

@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]

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]

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

[trjExpensify]

Perfecto, thank ya'll!