Show notice on save in site editor#36897
Merged
Merged
Conversation
tellthemachines
added
[Feature] Site Editor
|
Size Change: +98 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
getsource
reviewed
Nov 26, 2021
getsource
left a comment
getsource
left a comment
Member
There was a problem hiding this comment.
Thank you! This looks great.
I tested, and it works as expected.
In the below videos, I tested first by clicking the Save button twice, then by using CMD+S, followed by the return key, pressing the tab key after to see if the next focus targets would be the same. The next target for tab was the same both before and after applying the PR, as expected.
Before:
Before.Save.Notice.mov
After:
After.Save.Notice.mov
tellthemachines
force-pushed
the
add/notice-on-save
branch
from
965c1f6 to
1c2b3aa
Compare
tellthemachines
requested review from
ajitbohra,
nerrad and
ntwb
as code owners
kevin940726
reviewed
Nov 29, 2021
| ); | ||
| } | ||
|
|
||
| Promise.all( pendingSavedRecords ).then( ( values ) => { |
Member
There was a problem hiding this comment.
Could pendingSavedRecords fail? Might be easy enough to also add catch to handle it with error notices too.
Contributor
Author
There was a problem hiding this comment.
pendingSavedRecords is an array of the results of calling saveEntityRecord, which afaik never returns an error, just a record or undefined. That's why I'm checking for 'undefined' when showing the failure notice. Would it still be worth adding a catch in this case? We wouldn't really be doing anything with the caught error.
Member
There was a problem hiding this comment.
That's a bit unexpected TBH, why wouldn't saveEntityRecord throw an error if it... errors? I guess adding a catch case does no harm either, in case we want to revisit the behavior of saveEntityRecord.
Contributor
Author
There was a problem hiding this comment.
I'm not sure what the history behind this is, but the savePost function accounts for it by fetching errors in a super roundabout way. I'm guessing any changes to saveEntityRecord at this point would require a fair bit of refactoring anyway 😅
Not to say we can't add in a catch here, it won't hurt to do so. But we'll still have to generate the failure notices based on undefined as the return value too.
tellthemachines
force-pushed
the
add/notice-on-save
branch
from
2badc1d to
a40b80f
Compare
kevin940726
force-pushed
the
add/notice-on-save
branch
from
b2ffb98 to
bb324ba
Compare
Contributor
|
I merged a fix for the unit test issue - #36987 |
Contributor
|
@tellthemachines The notice is announced for me. It says "Site updated." Looks like it uses @wordpress/a11y.speak(). If some screen readers don't announce, you may need to force "assertive" as second parameter to that function. However, it seems to use this in the module you imported I guess, not directly. createSuccessNotice. |
tellthemachines
force-pushed
the
add/notice-on-save
branch
from
bb324ba to
5bbcc41
Compare
Contributor
|
Tested with NVDA on Windows in Chrome and Firefox. The successful update and the failure are announced in both browsers. |
kevin940726
approved these changes
Dec 14, 2021
kevin940726
left a comment
kevin940726
left a comment
Member
There was a problem hiding this comment.
Oh hey I also pushed some commits here so I obviously approve it 😆 . I'm not cheating.
tellthemachines
added a commit
that referenced
this pull request
Dec 21, 2021
* Show notice on save in site editor Co-authored-by: Kai Hao <kevin830726@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #36166. Adds notices on success and failure to the Save action in the site editor.
How has this been tested?
In the site editor, make changes to one or two template parts and click save. Check that a snackbar pops up when save is successful.
Make some more changes and set network to "offline". Saving again should cause an error notice to appear.
Repeat this process using a screen reader, and check that the notices are read by the screen reader when they pop up.
I tested this with VoiceOver in Safari, Firefox and Chrome. Safari and Chrome announce the notices correctly, but Firefox only announces the failure notice. It seems to ignore the snackbar altogether.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).