Styles Screen: Add link to global styles revisions #51149
Conversation
andrewserong
requested review from
noisysocks,
jameskoster,
jasmussen,
ramonjd and
tellthemachines
andrewserong
added
Global Styles
|
Size Change: +1.02 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
packages/edit-site/src/components/sidebar-navigation-screen-global-styles/index.js
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-global-styles/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-global-styles/index.js
Show resolved
Hide resolved
Thanks for testing, @ramonjd! I was a little stumped with this one until figuring out the |
There was a problem hiding this comment.
Code LGTM! Just a minor question below.
Also, in testing I noticed that if there are unsaved changes, the revision timestamp doesn't seem to move on from "a few seconds ago", though I don't think that's related to these changes.
Are we waiting on design review? Happy to approve otherwise 😄
packages/edit-site/src/components/sidebar-navigation-screen-global-styles/index.js
Outdated
Show resolved
Hide resolved
|
Oh that's cool work. GIF showing the revisions button under Styles:
I would love @jameskoster (back next week) and @SaxonF feedback if either has time, as I know they've worked a lot on these pieces, and are a better canonical source for design feedback. But from my angle, a few things: 
The revisions area should stick to the bottom closer to the save hub, even when there's scroll-space. Technically I don't know if it should be part of the save dock (Jay can probably speak better to this), but I suspect we'll want to surface buttons that look almost exactly like this one in a few other sections than just the Styles section, so there could be something to it. 
We can probably get a bit closer to Jay's design here:
The spacing is a bit tighter between borders, and there's no border above the revisions bar. |
|
Almost certainly separate, and related to conversations about modals and their initial focus management very probably, but this is a good example of why setting focus on the first interactive element inside isn't that useful: 
The close button gets way too much prominence when immediately focused like this, which isn't that useful since clicking it takes you here: 
Again, not blocking for this PR, but just an example of some of the modal conversations that I believe @ciampo has been looking into. |
Will keep it in mind when we'll start looking into refreshing the |
|
Thanks for taking this for a spin @jasmussen!
Good question. For implementation in this PR, I went with adding the button to the 
For this PR, would it be worth us merging this in as-is and follow up separately with how we want the |
👍 This is the right approach and was the reason for adding the footer prop.
It is a bit of a pain because the structure of the sidebar is a little messy but I think it's worth fixing in this PR. |
Thanks for confirming! I'm happy to work on it in this PR 👍 So it sounds like we want to:
I can chip away at that, so we've got something closer to Jay's design. |
That sounds right to me, but I'll let Saxon correct me if I'm off on this. |
| label={ __( 'Revisions' ) } | ||
| onClick={ onClickRevisions } | ||
| > | ||
| <HStack |
There was a problem hiding this comment.
Technically we need phrasing content elements inside button, so these should be spans. Also there are some existing divs inside SidebarNavigationItem - not introduced here.
|
Thanks folks! Update: I think I've managed to get the sticky footer area of the navigation screen to work correctly. I've pushed an update in cee7081. The changes involved:
Here's how it's looking now, along with the sticky footer behaviour: Styles Screen2023-06-05.16.38.31.mp4Page Details2023-06-05.16.39.12.mp4I think this is ready for another round of design feedback — is this closer to the intended result? And do the changes here cause any other issues across any of the screens? |
Co-authored-by: Ramon <[email protected]>
andrewserong
force-pushed
the
add/revisions-button-to-styles-screen-in-browse-mode
branch
from
f126caf to
cee7081
Compare
|
Flaky tests detected in e35f61b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5184113576
|
|
Stickiness is working well 👍 The dimensions are technically correct, and match other menu items. It looks a little off due to the left/right margins not aligning nicely with the save dock stroke, but that's a separate issue. The only (tiny) detail missing is the rounded corners. |
Thanks @jameskoster, I've updated the dimensions slightly so that it lines up with the save dock stroke — I think this should be the right position now, but we'll have a better idea of where it all sits once the update to the I've also added in the border radius using 
This PR should be ready for a final review I think now. Unless there are any blockers, I'd probably lean toward landing this PR in pretty close to its current shape now and looking at tweaks in follow-ups, since it looks like there are a few different styling things changing across multiple PRs. That said, happy to keep chipping away here if folks would prefer to wait for other PRs to land first 🙂 |
| @@ -326,6 +344,7 @@ function GlobalStylesUI() { | |||
|
|
|||
| <GlobalStylesActionMenu /> | |||
| <GlobalStylesBlockLink /> | |||
| <GlobalStylesRevisionLink /> | |||
There was a problem hiding this comment.
I think we might need to feed some more context to this hook. It seems to prevent switching between revisions and the stylebook.
2023-06-06.10.59.50.mp4
My assumption is that editorCanvasContainerView === 'global-styles-revisions' is already true so the panel doesn't switch.
Maybe we need something like canvasMode === 'edit' or something?
There was a problem hiding this comment.
Oh, good catch! I'll look into it, but yes, hopefully something like canvasMode === 'edit' might work.
There was a problem hiding this comment.
I think I've figured it out @ramonjd — it seems that the useEffect was firing correctly (only when the container is updated to global-styles-revisions), but that the Style Book button didn't appear to play nicely when the path was explicitly set to /revisions. That is to say, opening the Style Book doesn't update the path again. As a fix, I've updated this useEffect so that it also handles redirects for when the canvas container is updated to style-book, by redirecting to the root.
I believe this is needed because the style book button lives outside of the navigation provider for global styles, so it's probably good for consistency for us to handle the redirects in the one place.
I've updated in 51e26bc and 8ae9c7a — the latter ensures that we only redirect to the root path when we're on the revisions screen. I think the revisions screen is the only one where the style book won't work nicely, and this ensures that if someone is deeply nested within the global styles screens, opening the style book doesn't unexpectedly redirect back to root.
Let me know if you think this is an okay direction or if we should go in a different route... so to speak! 😄
There was a problem hiding this comment.
Let me know if you think this is an okay direction or if we should go in a different route... so to speak!
I'm sure we can navigate towards a good solution 
There was a problem hiding this comment.
Tested in Safari/Chrome/Firefox
Thanks for working on this and coming up with a nice footer solution!
If possible, it would be great to have a generic check for editorCanvasContainerView in GlobalStylesEditorCanvasContainerLink()... only if you think it matches the behavior you're after, otherwise LGTM.
| // redirect to the revisions screen. | ||
| goTo( '/revisions' ); | ||
| } else if ( | ||
| editorCanvasContainerView === 'style-book' && |
There was a problem hiding this comment.
Thanks for working this one out! I can now switch between views.
Do you think it would be okay to keep it generic, that is, if any container view is open? I'm wondering in case another container view is added down the road.
I tested and it appears to work for this situation.
| editorCanvasContainerView === 'style-book' && | |
| !! editorCanvasContainerView && |
There was a problem hiding this comment.
Good call! Yes, keeping it generic will help if and when we add any other container views as this logic should redirect away from the revisions screen if the container view is anything other than revisions and we're on the revisions screen 👍
That appears to work well to me, too, so I've committed in e35f61b with a slightly updated comment.
…e revisions screen. Co-authored-by: ramon <[email protected]>
Looks good to me, thanks for the suggestion and for re-testing! I've committed that. Once the tests pass, I'll merge this in and happy to continue with tweaks in follow-ups. |
andrewserong
deleted the
add/revisions-button-to-styles-screen-in-browse-mode
branch
Apologies, I should have been clearer in my last comment. The revisions button had the correct dimensions, it was the stroke on the save dock that we needed to update 🙈 No worries, I'll try to open a PR to address this. |
Thanks for opening up the follow-up PR! I left a comment on it, but the additional padding was added to ensure that the button lines up correctly with the save dock's new save button dimensions. It sounds like we might need to find a balance between aligning with other elements within the content area, and aligning with the save button 🤔 |
* Styles Screen: Add link to global styles revisions * Add button label * Update packages/edit-site/src/components/global-styles/ui.js Co-authored-by: Ramon <[email protected]> * Small code quality improvements * Try sticking the navigation screen footer to the bottom of the screen at all times * Add as span to the HStack * Update border radius and left padding so that the button lines up * Remove classnames call * Add redirect for style book, too * Only redirect to root when opening the style book on the revisions screen * Use truthy check for editor canvas container when redirecting from the revisions screen. Co-authored-by: ramon <[email protected]> --------- Co-authored-by: Ramon <[email protected]> Co-authored-by: ramon <[email protected]>
What?
Fixes #50429
Add a revisions button to the Styles screen in the site editor's browse mode. Clicking the button exits browse mode, and opens up the site editor proper, with the Revisions screen open.
Why?
As mentioned in #50429, it's helpful to provide quick access to the revisions for global styles.
How?
useEffectso that switching to the global styles revisions editor canvas container causes the path to be updated to navigate to the revisions screen. This follows similar behaviour introduced in Global Styles: Link to the block type panel when selecting a block with Styles open #49350 by @ntsekouras for linking to the block type panel. I tried a few different approaches, and this seemed the simplest, but happy for any feedback if folks think there are any issues with it!Testing Instructions
Questions for the reviewer
space-betweenbut I wasn't sure if more specific spacing was required.Last modifiedirrespective of the saved status of the current revision — so if changes have been made but not saved, the text is the same. Is this okay? We already have1 unsaved changenext to the Save button when there are unsaved changes, so I thought it could be redundant to swap outLast modifiedforunsaved🤔Testing Instructions for Keyboard
Check that the button on the Styles screen can be tabbed to. Is the label
Revisionsenough for this button?Screenshots or screencast
The Styles screen — note the addition of the
Last modifiedbutton / row at the bottom left:2023-06-01.14.04.37.mp4