Add __experimentalUpdateSpecifiedEntityEdits and use it on the Font Library update button #60390
Conversation
…lSaveSpecifiedEntityEdits in the font library update button
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @YanCol. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
Size Change: +27 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
matiasbenedetto
added
Global Styles
matiasbenedetto
changed the title
matiasbenedetto
added
the
[Type] Bug
There was a problem hiding this comment.
Confirming that this resolves the bug at #60343, following the reproduction steps there
- Existing custom styles aren't overridden when modifying fonts, before saving the editor
- Styles that have been changed in the editor but not yet saved are not persisted when modifying fonts
| // Get the base record from the datase to apply the edits to. | ||
| const base = select.getEntityRecord( kind, name, recordId ) || {}; | ||
|
|
||
| // Use the base record as the starting point for the edits. | ||
| const editsToSave = base; |
There was a problem hiding this comment.
| // Get the base record from the datase to apply the edits to. | |
| const base = select.getEntityRecord( kind, name, recordId ) || {}; | |
| // Use the base record as the starting point for the edits. | |
| const editsToSave = base; | |
| // Use the base record as the starting point for the edits. | |
| const editsToSave = select.getEntityRecord( kind, name, recordId ) || {}; |
There was a problem hiding this comment.
It looks like these lines are the only difference here from __experimentalSaveSpecifiedEntityEdits.
Is there a reason that __experimentalSaveSpecifiedEntityEdits doesn't use the existing entity as a base? If not, perhaps that function could be updated rather than creating a new, mostly duplication function?
cc @getdave @ntsekouras who had PRs that used __experimentalSaveSpecifiedEntityEdits somewhat recently.
There was a problem hiding this comment.
I thought about that, too, I decided to create a new one to be explicit about the difference with __experimentalSaveSpecifiedEntityEdits and avoid changing the existing functionality that used by other parts of the codebase too.
There was a problem hiding this comment.
Still, if we find that the behavior of __experimentalUpdateSpecifiedEntityEdits is the originally intended behavior of __experimentalSaveSpecifiedEntityEdits, we can add the changes from this PR to __experimentalSaveSpecifiedEntityEdits instead of creating the new function.
| * @param {Array} itemsToSave List of entity properties or property paths to save. | ||
| * @param {Object} options Saving options. | ||
| */ | ||
| export const __experimentalUpdateSpecifiedEntityEdits = |
There was a problem hiding this comment.
Just noting that we shouldn't be adding new experimental APIs anymore, we should add private APIs instead.
There was a problem hiding this comment.
Also it's not really clear what this API offers in addition to what saveEntityRecord does?
There was a problem hiding this comment.
saveEntityRecord saves all the changes to the database. __experimentalUpdateSpecifiedEntityEdits saves only the specified edits.
Let's take this case as example:
- Database has this content:
{ "a": "a"}- Client-side entity has:
{ "a": "a", "b":"b", "c":"c" }- We want to persist to the database only the following JSON without changing the client side entity:
{ "a": "a", "c":"c" }Using saveEntityRecord
saveEntityRecord( 'root', 'post-type', 1, { "a": "a", "b":"b", "c":"c" } )
{ "a": "a", "b":"b", "c":"c" } is persisted to the database.
Using __experimentalUpdateSpecifiedEntityEdits:
__experimentalUpdateSpecifiedEntityEdits( 'root', 'post-type', 1, [ 'c' ] );{ "a": "a", "c":"c" } is to the database.
Using __experimentalSaveSpecifiedEntityEdits:
__experimentalSaveSpecifiedEntityEdits( 'root', 'post-type', 1, [ 'c' ] );{"c":"c" } is to the database without changing the client-side entity.
I added these examples to the PR's description.
Probably the behavior added in this PR for __experimentalUpdateSpecifiedEntityEdits was the one originally thought for __experimentalSaveSpecifiedEntityEdits. In that case, we can move the additions to that function instead of adding a new one.
There was a problem hiding this comment.
__experimentalUpdateSpecifiedEntityEdits( 'root', 'post-type', 1, [ 'c' ] );
But in this case, can't we just send saveEntityRecord( 'root', 'post-type', 1, { "c":"c" } ) directly? In other words, can't we retrieve the edits that we want to save and just pass them (or not even mark them as edits). I'm pretty sure we do this already in other places already.
There was a problem hiding this comment.
__experimentalSaveSpecifiedEntityEdits to be honest I don't know much about this function, I need to check the history.
There was a problem hiding this comment.
This would create a new post revision only with { "c":"c" } content in the database instead of the desired { "a": "a", "c":"c" }.
I guess that it's possible that it's an endpoint issue then because if I'm not wrong all of the endpoints in Core work as "PATCH" (maybe not the global styles one, we need to check)
There was a problem hiding this comment.
It is also why we didn't see the issue before with __experimentalSaveSpecifiedEntityEdits (we never used it with the global styles endpoint before)
There was a problem hiding this comment.
Oh so you're actually trying to send only settings.typography.fontFamilies and not settings. That is going to override the whole "settings" object of course. So IMO, you should just send the update settings object entirely.
The endpoints don't do "nested patches", only top level.
There was a problem hiding this comment.
This would create a new post revision only with { "c":"c" } content in the database instead of the desired { "a": "a", "c":"c" }.
I don't think this is true for top level keys.
There was a problem hiding this comment.
This would create a new post revision only with { "c":"c" } content in the database instead of the desired { "a": "a", "c":"c" }.
I don't think this is true for top level keys.
That's correct in my testing for the global-styles endpoint, and true generally across the REST API. styles are preserved here when a font is activated because it's a different top level key, but settings are reset (without this PR) because they're empty except for settings.typography.fontFamilies.
|
Based on the discussion here #60390 (comment) I think we should change approach in this PR. We have two options: 1- Do not automatically persist activation of themes and just consider them like any other global styles changes. (Let the saving happen later with the save button) I personally prefer the first option because it's both simpler and more consistent with the rest of the UI. |
|
Thanks for the review.
I don't think there are two options. The 'auto-save' for the font families key is still needed because the auto-save functionality activates fonts just after they are installed.
I'll submit a new PR with that alternative. |
youknowriad
added
the
Backport to WP Minor Release
|
This PR is an alternative to this one: #60438 |
|
I'm closing this one in favor of the other PR. |
tellthemachines
removed
the
Backport to WP Minor Release
What?
__experimentalUpdateSpecifiedEntityEdits.__experimentalUpdateSpecifiedEntityEditsinstead of__experimentalSaveSpecifiedEntityEditsto persist changes to global styles with the Font Library,Why?
To allow update only some specified entity edits to the database.
Fixes: #60343
How?
Adding
__experimentalUpdateSpecifiedEntityEditsthat gets the content of the database as the base to apply certain edits.Example and comparisons:
Let's say we have a post and the following conditions.
{ "a": "a"}Client-side code made some changes to the data coming from the database.
{ "a": "a", "b":"b", "c":"c" }{ "a": "a", "c":"c" }Using
saveEntityRecord{ "a": "a", "b":"b", "c":"c" }is persisted to the database.Using
__experimentalUpdateSpecifiedEntityEdits:{ "a": "a", "c":"c" }is to the database.Using
__experimentalSaveSpecifiedEntityEdits:{"c":"c" }is to the database without changing the client-side entity.