Disable save changes button unless settings have changed #9386
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +409 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
While working on the Multi-Currency settings page today, I explored watching the data store for changes as trigger to enable the "Save Changes" button. This has a list of currencies that can become shorter or longer, so that seemed like a viable option. In the end it didn't work out because the list/data store is always change several times on init. So there's really no way to know what is the final loaded state of the list. Because of that, I opted to just manage state via the change events via the various form elements. I thought this was finished but realized there's one more page under advanced Fraud settings. |
mdmoore
requested review from
a team and
frosso
and removed request for
a team
There was a problem hiding this comment.
@mdmoore while reviewing, I noticed that most of the actions that modify the "main" settings go through the ACTION_TYPES.SET_SETTINGS_VALUES action.
Which means that the reducer for the settings can mainly rely on this action to determine if the settings have been modified or not.
This should simplify the implementation by just relying on a isDirty (or hasChanges) flag in the setting's Redux store.
For multi-currency, the values are coming from local state - so another hasChanges/isDirty local state can be leveraged to flag when exchangeRateType/manualRate/priceCharmType change.
The "fraud protection" settings are... A bit messier, NGL. So what you have here (with a new value in the context provider to set when changes have been made) makes sense.
However, it could be further simplified by removing the hasChanges from the context value, since hasChanges is not consumed by any of the consumers of the context.
I made the changes in this POC because it was easier to showcase than to explain :D
I think the main advantage is that it touches less files and leverages the existing behavior from the redux store, resulting in just shy of half of the file changes. And if a new setting gets added onto the data store (which is probably unlikely), we don't need to manually call the setHasChanges action.
I'd like to get your input on it!
| @@ -341,6 +344,8 @@ const FraudProtectionAdvancedSettingsPage: React.FC = () => { | |||
| setProtectionSettingsUI, | |||
| protectionSettingsChanged, | |||
| setProtectionSettingsChanged, | |||
| hasChanges, | |||
There was a problem hiding this comment.
I didn't notice hasChanges to be used by any of the consumers of the fraud protections rules context.
Can it be removed from the context provider's value?
There was a problem hiding this comment.
Thanks @frosso! This is extremely helpful.
I didn't notice hasChanges to be used by any of the consumers of the fraud protections rules context.
Can it be removed from the context provider's value?
Yes, thanks for catching it!
I noticed that most of the actions that modify the "main" settings go through the ACTION_TYPES.SET_SETTINGS_VALUES action.
Which means that the reducer for the settings can mainly rely on this action to determine if the settings have been modified or not.
This should simplify the implementation by just relying on a isDirty (or hasChanges) flag in the setting's Redux store.
This is definitely preferred and what I was attempting with the multi-currency settings via the SET_CURRENCIES action. I abandoned that approach due to what I mentioned here. Basically, SET_CURRENCIES is the action dispatched when adding or removing currencies. But this action is dispatched as the component loads as well, so it wasn't reliable. I see that's not the case for the main settings, so that will work! I'll give this another look in the morning.
|
@frosso I was able to address your suggestions for the most part. Main Settings Multi-currency Main Currency Settings Fraud Settings |
mdmoore
force-pushed
the
update/9305-settings-disable-save-changes-button
branch
from
3f69a94
to
0554da1
Compare
There was a problem hiding this comment.
Single Currency Settings
The single currency settings( clicking "manage" on any currency ) now use a local state. I was seeing some strange behavior when setting the state via the useEffects you recommend [...] For this reason, I opted to set the state the individual onChange props.
Ah, gotcha - it does seem like I didn't account for something, in there.
That sounds good, using local state makes it more explicit :)
Main Currency Settings
A local state works for the Store Settings section but the context is needed in order to react to currencies added via the modal as well as clicks on the delete button. It's not exactly necessary to enable the save here since added and removed currencies are already saved, however it feels strange to have a save button for some changes and not others. What's your opinion on that?
It does seem that the "save changes" button is needed only for the "Store settings" section of the multi-currency screen.
This is the behavior I'm seeing:
Screen.Recording.2024-09-18.at.9.12.45.AM.mov
I think it could be a double-edged sword: on one hand, if a merchant adds/removes some currencies and the button becomes enabled, they might think that their changes haven't persisted and they could safely exit the page without having made any changes; on the other hand, it does make the page's behavior less consistent.
I don't think it's something we need to fix in this PR - the inconsistent behavior has been present for a long time.
But I'll point out that there are pages in WooCommerce that persist the changes immediately, keeping the "save changes" button in a disabled state.
The one that comes to mind is the page to manage shipping zones. When a new shipping zone is added/removed, the changes are persisted immediately.
Screen.Recording.2024-09-18.at.9.18.31.AM.mov
Whether that behavior is wanted or not, I think the multi-currency page would behave consistently with the shipping zone page if we keep the button disabled when a currency is added/removed :)
There was a problem hiding this comment.
Thanks for entertaining my suggestions! I took a second pass at the code as well, looks good! Approving, since the other stuff is optional and up for debate :)
I did notice that the browser's "Changes you made may not be saved." dialog was still displayed even after the "save changes" button got disabled, but I don't think it was in the acceptance criteria.
multi-currency/client/context.js
Outdated
| const MultiCurrencySettingsContext = createContext( { | ||
| isDirty: false, | ||
| setIsDirty: () => null, | ||
| } ); |
There was a problem hiding this comment.
(minor) it looks like these were added to make TS happy - maybe we should also add the other possible context values for consistency? (isSingleCurrencyScreenOpen, currencyCodeToShowSettingsFor, openSingleCurrencySettings, closeSingleCurrencySettings)
There was a problem hiding this comment.
I've emptied these out now, since we'll no longer use them 👍
This is a good callout. I think this is a strong enough reason to leave it disabled for the already persisted changes. Your point on matching the behavior of other similar cases with settings helps as well. We can go with a local state and only worry about the "Store Settings".
I noticed this as well. I looked into it and wasn't able to find a quick to solution to implement alongside the save button changes. It seems like adding a |
mdmoore
deleted the
update/9305-settings-disable-save-changes-button
branch
Fixes #9305
Changes proposed in this Pull Request
This ensures that the "Save Changes" button on WooPayments settings pages starts off disabled and only becomes active once a change is made, following the pattern in WooCommerce core settings. This functionality is achieved by managing a state variable that tracks whether any form elements have been modified. The hasChanges boolean is set to true whenever a form field is altered, enabling the Save button accordingly.
The PR includes a lot of changed files so it might be easier to review via each commit.
The settings pages changed are:
Testing instructions
In general, testing should involve checking that the Save changes button is disabled by default, then testing that it becomes enabled when any setting is changed. This touches every setting so I think it's important to test all of them. The risk with missing one is that the settings couldn't be saved, which could be confusing to merchants.
WooPayments Settings
Check that the save button is enabled after changing each form field. The page will need to be refreshed after each change.
Advanced Fraud Protection Settings
Multi-currency Settings
Express Checkouts
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge