-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global styles: enable user global styles revisions #46667
Conversation
475a386
to
e32c7d4
Compare
Size Change: +1.9 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
0a8dfd1
to
777738b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is adding a revisions property to the global styles response a good idea? Should we rather add a new entry to _links and add a separate endpoint for revisions to avoid huge responses?
Are there any scenarios where we might want to fetch global styles (or part of it) without the revisions? If so, yeah it's probably better to have a separate endpoint for them.
Testing this a bit, it's working pretty well for under 10 revisions, but started acting funny once I added a bunch of them in a row (I didn't always reload the page between saves, not sure if that might have had any influence).
As mentioned below, only the last 10 revisions are appearing in the dropdown.
I was switching between style variations in TT3, and ended up with all the variations showing gradient backgrounds:
This happened when I tried to go back to a previous revision after switching to the sherbet variation and fiddling with a bunch of block styles.Also, changes to block global styles weren't reverted when going back to a previous revision.
lib/compat/wordpress-6.2/class-gutenberg-rest-global-styles-controller-6-2.php
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/screen-revisions.js
Outdated
Show resolved
Hide resolved
Good pick up, thank you! I thought I saw that too but was hoping that it might have been also happening on trunk. Turns out hope is a dangerous thing :) |
3f1543a
to
e0753cb
Compare
It actually is occurring on trunk. Steps to reproduce:
2022-12-23.14.25.12.mp4
Issue created: #46758 |
fcca8f6
to
7ac4391
Compare
@@ -453,7 +453,7 @@ __experimentalGetTemplateForLink.shouldInvalidate = ( action ) => { | |||
); | |||
}; | |||
|
|||
export const __experimentalGetCurrentGlobalStylesId = | |||
export const __experimentalGetCurrentGlobalStyles = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I'm returning the entire global style object instead of just the ID is so we can access the revisions url.
E.g.: currentGlobalStyles?._links?.[ 'version-history' ]?.[ 0 ]?.href;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed a waste not to repurpose this method since it was fetching what we wanted anyway and only returning the id.
Plus it's experimental so I didn't see much harm in changing it.
Some questions from the design perspective:
cc @WordPress/gutenberg-design |
…ating the full revision. The revision should overwrite everything.
- same behaviour as post editor - since revisions includes the current saved post, so we need at least two in order to switch
…yet delete global styles revisions and, because of other tests, we can't rely on the post revisions being a certain count so we try to cater for both in the test for now. Added aria label to buttons in the UI
Checking for the number of revision buttons in the revisions list.
…rentGlobalStylesId, which has been removed in this PR. Adding a reset button.
Removing secondary button styling
Updating e2e tests A sad attempt at caching the unsaved userConfig Adding an `is_latest` prop to the rest response to indicate the latest saved revision
17f9ba6
to
9975e1a
Compare
Redesigning the buttons view Rationalizing rest API return properties
9975e1a
to
408a755
Compare
I've been running around in circles in an attempt to come up with a reasonable workflow today, and have concerns about how to deal with unsaved changes in the editor. I'm worried that users won't know that loading a revision overwrites the styles in the editor canvas, thereby removing any unsaved changes. For example, a user might want to browse previous revisions only. @jameskoster raised this concern at the start of this PR, and suggested that a warning might be temporary approach until it's possible to load revision styles independent to the styles that show in the editor. This is what I've come up with following those lines: 2023-04-18.16.35.13.mp4I haven't committed it to this PR yet. Thinking more broadly, I have positive feelings towards an in-editor revisions view, let's say something like the style book where we could compile and load any style without affecting the main editor canvas. That would require a major refactor, but it might be useful to serve as a basis for comparing template revisions as well. I'd like to explore this option in hack branch. |
Yeah this feels like a blocker to me. I know we have the Undo affordance, but it doesn't feel intuitive in this case. What you shared above seems like a good start. The action(s) are significant enough that it warrants being placed in a modal imo. We could potentially invoke a similar UI in other flows where customisations are lost such as switching variations: #40561 |
Thanks folks. @andrewserong and @tellthemachines had some good ideas that I can trial as well: namely, auto-saving unsaved changes (if any) when opening the revisions panel. This would technically create a new revision I think, but I'm interested to see how it would work in practice.
This makes perfect sense too. I'll play around. |
It was all @tellthemachines' idea, I was just being the cheer squad 😄 Thanks for digging into this feature so deeply! |
My plan is to implement this in this PR, but I took a stab at repurposing the stylebook code to load blocks and styles independently. The main benefit is that users can open revisions, try them out and return to the editor in whatever state they left it in, that is, any unsaved changes are unaffected. There's still the need for a prompt to save unsaved changes before a revision is restored just in case. |
Closing this PR and tracking follow ups based on the experimental PR over in the issue: |
Related to:
What?
Display revisions to a user's global style changes in the global styles sidebar (revisions for the
wp_global_styles
custom post type).These revisions are the ones made by the current, logged-in author. (should we show all authors' revisions?)
We're only showing maximum 100 to limit the payload.
2023-04-12.16.50.24.mp4
How?
By:
'version-history'
entry to_links
, mimicking the one from the posts API that containscount
andhref
properties/revisions
, which also mimics the REST API's/wp/v2/posts/<parent>/revisions
– that returns data fromwp_get_post_revisions()
Testing Instructions
npm run test:unit:php -- --filter Gutenberg_REST_Global_Styles_Controller_Test
npm run test:e2e:playwright -- --headed site-editor/user-global-styles-revisions.spec.js