Reset error key on settings first load. #9410
Conversation
benbowler
force-pushed
the
bug/8481-clear-settings-error-border
branch
from
1139dde
to
2151b7e
Compare
benbowler
changed the base branch from
enhancement/8747-subtle-notification-formatting
to
develop
benbowler
force-pushed
the
bug/8481-clear-settings-error-border
branch
from
2151b7e
to
78df35b
Compare
|
Build files for e6f1d56 have been deleted. |
|
Size Change: +5 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @benbowler, but it looks like we should in fact stick with the IB here. Please see my comment for details.
| // Reset settings panel errors when the component first loads. This is | ||
| // necessary to remove the red error border when returning from the | ||
| // OAuth flow after allowing additional scopes. | ||
| useEffect( () => { |
There was a problem hiding this comment.
Taking a look at the AC, it seems we should in fact stick with the approach in the IB.
The settings error appearance should be removed as soon as the settings submission completes successfully
Note that the settings submission will only complete once Confirm changes has been pressed, so the IB should fulfil the AC as they stand. Please can you revert this change and apply that specced in the IB?
Incidentally, while looking into this and testing other error scenarios, I discovered a regression has crept into the Analytics submitChanges() action:
site-kit-wp/assets/js/modules/analytics-4/datastore/settings.js
Lines 160 to 162 in 78df35b
The return statement above should be as follows:
return { error };At the moment, if an error occurs while saving the Analytics settings on the Settings edit screen, it will be silently ignored.
Please can you also roll this fix into this PR?
There was a problem hiding this comment.
Ah interesting, the UX didn't seem clear to me when I was testing it but I can see how the error state doesn't need to be updated until the user tries to submit the settings again.
benbowler
force-pushed
the
bug/8481-clear-settings-error-border
branch
from
78df35b
to
e6f1d56
Compare
There was a problem hiding this comment.
Thanks @benbowler, this LGTM. The QAB needed a tweak, which I applied myself to save a round trip.
Summary
Addresses issue:
Relevant technical choices
The implementation in the IB didn't quite work because it wouldn't remove the red border until the user clicked the confirm button again after returning to the page. Instead I use a useEffect to reset this error key on first load which removes the border when returning from the OAuth flow.The implementation reverted back to the IB as discussed here: #9410 (comment)
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist