-
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: add revisions UI #50089
Conversation
Size Change: +2.7 kB (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
Great progress! @jasmussen @jameskoster @SaxonF — one thing we should be getting ahead of is treating "styles" as a more comprehensive section where revisions, style book, etc, can feel more integrated. Thinking of the ways in which we present the header and header tools, for example. |
b7593d4
to
68dc212
Compare
68dc212
to
aed45a2
Compare
Flaky tests detected in 0774936. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4910136647
|
lib/experimental/class-gutenberg-rest-global-styles-revisions-controller.php
Outdated
Show resolved
Hide resolved
👋 @jameskoster I've got this branch working according to the last version of my explorations. The way I've set it up means that we can load and preview revisions without affecting the editor state and, therefore, any unsaved changes. Just wanted to check how far we could take things according to the timeline demo you posted. I like the time line look, and I'd like to take a stab at it. I just had a couple of thoughts: ❓ Would the number of revisions impact the layout? What if there are > 50? I'm loading them in a select dropdown for now when the number of revisions is > 10, which is practical, but doesn't look fabulous. ❓ Is the save modal still warranted? It might be a tricky where there are template and global styles changes. I assume we'd only want to save global styles and not template changes. I think it'd be beneficial to do that in a follow up PR given the complexity, for ease of review. I've already mentioned it, I believe we don't need to introduce a new save flow now that previewing revisions is independent of the editor. 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. Thanks! |
a6dd7f0
to
6952aa1
Compare
Oh, nice, that is excellent news! That probably means we don't need the save modal, but may need a dedicated 'Load this revision' button which my designs didn't have. I'll revisit my prototype and share an update soon.
I don't think so. Any reason not to just allow the timeline to scroll? |
Storing unsaved changes makes for a much simpler design :) revisions.mp4
Until Styles exists as a more comprehensive section as mentioned above, we'll need to think about what happens when a user selects a revision, doesn't load it, then exits the Revisions timeline somehow (clicks back, closes Styles panel, opens Inspector, etc). In other words, what happens if you click the I think it's probably fine to just return to the stored unsaved changes for now, but it feels like a situation where it could be good to get confirmation from the user. |
test/e2e/specs/site-editor/user-global-styles-revisions.spec.js
Outdated
Show resolved
Hide resolved
test/e2e/specs/site-editor/user-global-styles-revisions.spec.js
Outdated
Show resolved
Hide resolved
test/e2e/specs/site-editor/user-global-styles-revisions.spec.js
Outdated
Show resolved
Hide resolved
test/e2e/specs/site-editor/user-global-styles-revisions.spec.js
Outdated
Show resolved
Hide resolved
6952aa1
to
7ecfd1d
Compare
You know what, I don't know! 😄
Thanks a lot for taking the time to revisit the design.
@jameskoster I've made pretty decent headway on the design today. See the updated GIF in the PR description. Code-wise there's a bit to clean up, including splitting out API and other changes that might require separate reviews. Also, ignore the pesky CI test fails... I'll deal with them later 😜 |
Documenting follow ups:
|
Hi, @ramonjd The E2E tests for this feature seem to be flaky -https://github.com/WordPress/gutenberg/issues?q=is%3Aissue+is%3Aopen+in%3Abody+user-global-styles-revisions.spec.js. |
Thanks @Mamaduka I'll check it out 👍 |
It seems like the issue is that we're not disabling the Styles welcome guide on the first load. So it always fails once but passes the next run. It's easy to miss, but there's an automatically-created issue in #50089 (comment). Also there were inline failure messages in the File tab too (L60 and L117). |
Thanks @kevin940726!! Looks like I forgot to pass Edit: Oh, now I remember - I saw that That's why I left it. 🤷 Edit Edit: You said the styles welcome guide. I understood the other one, sorry. 🤦 |
I have what I hope is a fix for the flakiness over at #50454 Turns out I was leaving unsaved changes in the site editor so I think the test might have been trying to select an already-saved style. I don't know. But it appears to run more reliably locally now. |
@ramonjd one thing we missed here that would be good to address in a follow-up: Use the zoom-out view for the canvas, rather than the stylebook container. |
Sounds like a good follow up. Thanks @jameskoster 👍 I'll make a note. |
Related to:
What?
This PR displays revisions to the global styles in the global styles sidebar (revisions for the wp_global_styles custom post type) in a timeline-like list.
At the moment is a limit imposed by the rest controller of 100 revisions.
The revisions panel will only be available once there are more than two revisions. This is because the theme global styles default state is not treated as a revision. It's possible to add it in though.
How?
Building on the work in :
TODO
Testing Instructions
Run the E2E tests if you want
Screenshots or screencast
2023-05-04.10.59.37.mp4