Directly save if only changing current context #50567
Merged
Conversation
This file contains 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
github-actions
bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @SaxonF! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
Size Change: +419 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
This feels very natural to me. The running total of changes on the button label helps create a sense of urgency as changes stack up which seems appropriate. And I particularly appreciate not having to click the save button twice in the local context. A small detail: when saving via the modal, it's a bit strange how the button in the sidebar immediately changes to the disabled/busy state with the "No changes to save" label. Especially if there's an error... the aforementioned label would not be true. Should there be an intermediary state, IE Something to look at separately, but that busy state could perhaps use a visual iteration. |
|
Awesome to see this evolve! Some quick feedback: I find "no changes to save" to be strange phrasing. I think a classic "Saved" works, similar to what we have in the post editor: saving.mov |
|
Flaky tests detected in e147c67. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5185269450
|
|
Changed back to "Saved" and button now says "Saving" when saving is in progress.
Definitely. The save button is currently is in both side editor sidebar and toolbar so wary of changing too much there. Let's create a separate issue to refine. |
|
Here's a funny one, if you edit a menu in the Navigation panel you don't get the local "Save" button, but instead a "Review changes" one: 
I suspect that may be due to the unique handling of navigation menus, but it would be nice to button up if not too difficult. |
youknowriad
reviewed
May 15, 2023
youknowriad
reviewed
May 15, 2023
noisysocks
reviewed
Jun 6, 2023
noisysocks
reviewed
Jun 6, 2023
Comment on lines
+98
to
+100
| if ( isSaving ) { | ||
| label = __( 'Saving' ); | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
It does but we also need it for the direct saving button
noisysocks
reviewed
Jun 6, 2023
| if ( disabled ) { | ||
| return __( 'Saved' ); | ||
| } | ||
|
|
||
| if ( defaultLabel ) return defaultLabel; |
There was a problem hiding this comment.
Not a huge deal since SaveButton isn't a component that we export to third parties but I think the semantics around defaultLabel are a little confusing. (When will it be shown?) If possible I think it'd be better to have a label prop that allows callers to override the label altogether. That way it's clearer that when label is set then that's what will appear.
There was a problem hiding this comment.
@noisysocks main reason I called it defaultLabel was that the button label changes depending on saving state. If we called it label we'd have to also provide labels for each state (e.g. disabled, saving etc)
noisysocks
approved these changes
Jun 6, 2023
ndiego
added
[Type] Enhancement
43 tasks
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
This PR updates the behaviour of the save hub in the site editor so that it only prompts to review changes when there are changes outside the current context. It also adjusts some styling.
Why?
As part #50379 we have been exploring how to improve the save dock. This is a proposal to do just that by adjusting behaviour and making some small style changes.
How?
Checks to see if there is only one record to save and if that matches the current context based on url params.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
save-pr-demo.mp4