[Site Editor]: Consolidate save button functionality #60077
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
ntsekouras
added
[Type] Bug
ntsekouras
requested review from
draganescu,
mcsf,
Mamaduka,
youknowriad,
jameskoster and
SaxonF
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ntsekouras
commented
Mar 21, 2024
| export default function useSaveEntities( { | ||
| onSave, | ||
| entitiesToSkip = [], | ||
| close, |
There was a problem hiding this comment.
This doesn't seem right, but I think it's fine for now and handle in follow ups because it will require more disentanglement of how to properly call, name, etc..
There was a problem hiding this comment.
What's preventing this hook to be an action instead?
There was a problem hiding this comment.
Probably nothing. I can check and change.
There was a problem hiding this comment.
Updated to action
ntsekouras
commented
Mar 21, 2024
| return true; | ||
| } | ||
| return false; | ||
| return select( coreStore ).hasEditsForEntityRecord( |
There was a problem hiding this comment.
Not related, but had to change it since I saw it 😄
ntsekouras
commented
Mar 21, 2024
| variant={ isDisabled ? null : 'primary' } | ||
| showTooltip={ false } | ||
| icon={ isDisabled && ! isSaving ? check : null } | ||
| showReviewMessage |
There was a problem hiding this comment.
This is the prop I mentioned in the PR's description to show a different message if there are more entities to update.
The defaultLabel which I removed, was introduced and used for this specific case only.
|
Size Change: +319 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
ntsekouras
removed
the
[Type] Code Quality
jameskoster
approved these changes
Mar 21, 2024
Mamaduka
reviewed
Mar 22, 2024
|
Flaky tests detected in fbd63db. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8388302003
|
ntsekouras
force-pushed
the
consolidate-save-button-usage
branch
from
97dc4eb to
e4b495e
Compare
Mamaduka
reviewed
Mar 22, 2024
Mamaduka
reviewed
Mar 22, 2024
Mamaduka
reviewed
Mar 26, 2024
There was a problem hiding this comment.
Thanks for the follow-ups, @ntsekouras!
Do you mind adding more detailed testing instructions? Because unless I'm missing some obvious step, the Site Editor's saving flow works as before for single entities.
Screencast
CleanShot.2024-03-26.at.11.13.01.mp4
| * @param {Function} [options.close] Callback when the actions is called. It should be consolidated with `onSave`. | ||
| */ | ||
| export const saveDirtyEntities = | ||
| ( { onSave, dirtyEntityRecords = [], entitiesToSkip = [], close } = {} ) => |
There was a problem hiding this comment.
The callback argument seems a bit odd to me. This is a pattern I've rarely seen with store actions. Do you think consumer components can handle them? Also, consumers should have all the data to generate the entitiesToSave array.
There was a problem hiding this comment.
For sure close should be consolidated probably with the onSave and also see what will need more changing, but I was thinking of doing it separately as this is already moving a lots of stuff around and this action is private.
The end goal is the consolidation of publishing flows between editors, and this in essence is a first step towards that, by removing an extra save button.
There was a problem hiding this comment.
Makes sense. Let's include cleanup/refactoring in the follow-up PR(s).
Comment on lines
+126
to
+128
| const PUBLISH_ON_SAVE_ENTITIES = [ | ||
| { kind: 'postType', name: 'wp_navigation' }, | ||
| ]; |
There was a problem hiding this comment.
Nit: I think we can move this constant at the file level.
This is true only for the home template. If we're editing any other entity it doesn't require two steps. For this PR I started with an approach of checking the |
Mamaduka
approved these changes
Mar 26, 2024
There was a problem hiding this comment.
Thanks for the clarification, Nik!
I can confirm that single-step saving works as expected for entities other than the Home template.
I think the PR is in good shape as a starting point to consolidate the functionality. Let's polish the components and actions in follow-ups.
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
Co-authored-by: ntsekouras <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: SaxonF <[email protected]> Co-authored-by: richtabor <[email protected]>
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
Co-authored-by: ntsekouras <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: SaxonF <[email protected]> Co-authored-by: richtabor <[email protected]>
|
Just wondering if anyone who worked on this PR gets a chance, if you can take a look at the issue raised in #60650? It looks potentially related as this appears to have changed the logic for the activation button when users live preview a theme and then go to activate it, but haven't made any other changes to the state. 🤔 |
1 task
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?
Fixes: #60058
Currently we have many
savebuttons and some functionality is duplicated and adds complexity to the code. The save button in navigation sidebar sometimes uses the save button of document header and sometimes a custom one. When using the custom one, we almost duplicate the saving of the entities callback from thesavebutton inEntitiesSavedStatespanel.This PR refactors the code:
review x item..message, but we could also consolidate as well at some point(or here).EntitiesSavedStatespanelThe goal is to try to consolidate related components that will also help to the unification of publishing flows between post and site editors(issue).
Testing Instructions