Site editor: add global styles changes to save flow #57470
Conversation
|
Size Change: +215 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
update/display-global-style-changes-save-flow
branch
2 times, most recently
from
de963ec
to
3335712
Compare
ramonjd
added
[Type] Enhancement
ramonjd
changed the title
There was a problem hiding this comment.
When I make changes via global styles and hit save, this works perfectly, no issues there.
When I click and apply a previous revision and hit save, the items there don't seem to line up with the things that changed in the revisions list. I'm guessing that's actually correct though since it's comparing the one applied to the revision previous to that, not the current active revision. Is that right?
|
Thanks for testing @apeatling 🙇🏻
If something is added or taken away, it will count as a change. I've been playing around and what I suspect we're seeing is the change list reporting the removal of the same items that we'd previously saved. Maybe it would be clearer for users to have something like "Updated/Added/Removed" sections - that could involve a bit of refactoring of the change detection method I think, but theoretically doable. |
An alternative could be to just have some preceding copy, e.g., |
|
Yeh, let's add "Changes made to:" and see how that looks, it might be clearer. |
ramonjd
force-pushed
the
update/display-global-style-changes-save-flow
branch
from
04f974c
to
6e918b9
Compare
Updated. Thank you! |
There was a problem hiding this comment.
This is looking good to try to me!
Most of the time when I make an update, each of the changes will be included as in:
I had to make quite a lot of changes before hitting the cap of 10, so that limit seems reasonable to me:
✅ Existing descriptions are displayed when they should
✅ Global styles includes "Changes made to:" list and is testing well
The only tiniest of nits I noticed is that the overall panel's "Are you ready to save" text uses a line-height of 1.5, applied automatically as part of the p tag:
The panel row gets a line-height of 1.4em since it's just text in a div instead of being a paragraph:
That's not at all a blocker to landing, but could be something to tweak if it's unintentional that we have different line-heights there. On the other hand, since the panel text is a bit more data-ey for each of the things that has changed, it might make sense to have the narrower line-height (in which case we could leave it anyway).
LGTM! ✨
|
Thanks for testing this one! |
Good spotting. Happy to change if it's an eyesore. |
|
Nice work. Visually this could be a bit stronger, though. Here's a before/after with some suggestions:
Main differences:
@ramonjd would you be up for a quick followup for the above pieces? There's a bit more, that's not directly related to this PR, so you can pick and choose whether to include or omit:
Note that the above sketches consider the work that is being proposed in #49832 (comment) as well. |
|
Thanks for the feedback @jasmussen! I'll get a PR going we can test with your suggestions. |
|
A preliminary PR is up: It takes care of the following:
And also updates the "ready to save" copy from The items are definitely doable:
They require a bit more fiddling so I can do separate follow ups. For example, changes to the global styles changes list will also affect the revisions summaries. I don't think they're controversial, but it would be good to test the change independently. The "time since the change" copy relies on the "modified" field for each entities. The global styles controller doesn't yet return the global styles post's 'modified' field. To be consistent I think we should enable it. More to come... |
|
I think I misunderstood this at first. I think you mean the "time since unsaved change"? Not the time since the record was last modified in the database? If the former, then I suspect that would be a little more verbose to implement. My guess is that we'd save the datetime every time an entity is "edited". I'm looking at the core data actions: gutenberg/packages/core-data/src/actions.js Line 380 in 4ea4431 Assuming it's possible, it might take a PR or two of preparatory work to get there. For the heck of it, I tested out the UI changes nevertheless, and, assuming the data is there, it'd require a small update to the entity-record-item.js component. 
Something like <PanelRow>
<CheckboxControl
__nextHasNoMarginBottom
label={
<strong>
{ decodeEntities( entityTitle ) || __( 'Untitled' ) }
</strong>
}
checked={ checked }
onChange={ onChange }
/>
<span>{ humanTimeDiff( entityModified ) }</span>
</PanelRow> |
|
Thanks for all the work. I also left a comment on the other PR.
Right, it's meant to be a subtle reminder that these change can stack up across a site editor session, you might have made an accidental change somewhere in a template, just tinkering, and then forgetting about it. This would be there to say: "this change you made an hour ago", to help make you decide whether to include it in the save or not. I believe @jameskoster instigated this concept, and it would be a nice enhancement to look at. But worth noting, I wouldn't consider it urgent at all, and it's even something we can live without, if need be. |
What?
Follow up to:
Part of:
This PR prints current, unsaved changes to global styles in the save flow panel of the site editor.
It uses the same comparison code as the global styles revisions change summary.
Why?
To provide the user with an overview of what's being saved, so, if I had to write a sales pitch I'd focus on "transparent and informative" as adjectival hooks, and then offer a free set of steak knives if that didn't work.
How?
By comparing the current user global styles object, which includes any saved and unsaved changes to global styles, with the saved record. The difference should reflect the unsaved changes that will be saved in the save flow.
Open questions
…and n more changes.appears to communicate the existence of more. Is that limit reasonable?"Your unsaved changes include: ...."?Testing Instructions
Also check that the
<h3>Changes made to:</h3>order makes sense in context:Screenshots or screencast
2024-01-09.11.34.20.mp4