Fix : Global styles revisions close button overlaps scrollbar #66456
Conversation
|
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 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. |
There was a problem hiding this comment.
Thanks for trying this out! I think the header space can work. Before we venture further it would be good to make sure @WordPress/gutenberg-design would agree it’s a good enough solution.
As for the current implementation in this branch—like you said, it doesn’t seem ideal to use the aria-label in a selector. Also, putting this revision-specific ruleset in the styles of EditorCanvasContainer seems unideal.
I think to do this more cleanly EditorCanvasContainer needs to support a className prop and the consuming components can leverage it for style overrides.
Here’s a diff where I started down that path and decided the Revisions doesn’t need to override the base close button’s styles but rather Style Book can override them. Also I didn’t stay too focused as there are some things in style book that could use some cleanup. So I'm adding this largely as a note that we could make a separate focused PR on that Style Book cleanup. The other changes seem like a good direction for this issue given the design change is favored of course.
diff --git a/packages/edit-site/src/components/editor-canvas-container/index.js b/packages/edit-site/src/components/editor-canvas-container/index.js
index ac1083e69a..de687128b9 100644
--- a/packages/edit-site/src/components/editor-canvas-container/index.js
+++ b/packages/edit-site/src/components/editor-canvas-container/index.js
@@ -1,3 +1,8 @@
+/**
+ * External dependencies
+ */
+import clsx from 'clsx';
+
/**
* WordPress dependencies
*/
@@ -46,6 +51,7 @@ function getEditorCanvasContainerTitle( view ) {
function EditorCanvasContainer( {
children,
+ className,
closeButtonLabel,
onClose,
enableResizing = false,
@@ -114,7 +120,12 @@ function EditorCanvasContainer( {
return (
<EditorContentSlotFill.Fill>
- <div className="edit-site-editor-canvas-container">
+ <div
+ className={ clsx(
+ 'edit-site-editor-canvas-container',
+ className
+ ) }
+ >
<ResizableEditor enableResizing={ enableResizing }>
{ /* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */ }
<section
diff --git a/packages/edit-site/src/components/editor-canvas-container/style.scss b/packages/edit-site/src/components/editor-canvas-container/style.scss
index 332c4cd123..9ea4fb2df8 100644
--- a/packages/edit-site/src/components/editor-canvas-container/style.scss
+++ b/packages/edit-site/src/components/editor-canvas-container/style.scss
@@ -17,27 +17,16 @@
.edit-site-editor-canvas-container__section {
background: $white; // Fallback color, overridden by JavaScript.
border-radius: $radius-large;
- bottom: 0;
- left: 0;
+ height: 100%;
overflow: hidden;
- position: absolute;
- right: 0;
- top: 0;
transition: all 0.3s; // Match .block-editor-iframe__body transition.
+ display: flex;
+ flex-direction: column;
}
.edit-site-editor-canvas-container__close-button {
- position: absolute;
- right: $grid-unit-10;
- top: $grid-unit-10;
- z-index: 2;
- background: $white;
-
- // Prevents Scrollbar overlapping the close button when viewing revisions.
- &[aria-label="Close revisions"] {
- position: relative;
- top: 0;
- margin: 8px 0;
- left: calc(100% - #{$grid-unit-50});
- }
+ flex: none;
+ align-self: flex-end;
+ margin-inline: 0 $grid-unit-10;
+ margin-block: $grid-unit-10;
}
diff --git a/packages/edit-site/src/components/revisions/index.js b/packages/edit-site/src/components/revisions/index.js
index 5ce95872e0..2b5f9f81fc 100644
--- a/packages/edit-site/src/components/revisions/index.js
+++ b/packages/edit-site/src/components/revisions/index.js
@@ -72,6 +72,7 @@ function Revisions( { userConfig, blocks } ) {
title={ __( 'Revisions' ) }
closeButtonLabel={ __( 'Close revisions' ) }
enableResizing
+ className="edit-site-revisions"
>
<Iframe
className="edit-site-revisions__iframe"
diff --git a/packages/edit-site/src/components/style-book/index.js b/packages/edit-site/src/components/style-book/index.js
index 93bbc6311c..f8ae7fc1b0 100644
--- a/packages/edit-site/src/components/style-book/index.js
+++ b/packages/edit-site/src/components/style-book/index.js
@@ -200,12 +200,10 @@ function StyleBook( {
onClose={ onClose }
enableResizing={ enableResizing }
closeButtonLabel={ showCloseButton ? __( 'Close' ) : null }
+ className="edit-site-style-book"
>
<div
- className={ clsx( 'edit-site-style-book', {
- 'is-wide': sizes.width > 600,
- 'is-button': !! onClick,
- } ) }
+ className="edit-site-style-book__liner"
style={ {
color: textColor,
background: backgroundColor,
diff --git a/packages/edit-site/src/components/style-book/style.scss b/packages/edit-site/src/components/style-book/style.scss
index 773b4f10f5..7347325d15 100644
--- a/packages/edit-site/src/components/style-book/style.scss
+++ b/packages/edit-site/src/components/style-book/style.scss
@@ -1,14 +1,16 @@
.edit-site-style-book {
// Ensure the style book fills the available vertical space.
// This is useful when the style book is used to fill a frame.
- height: 100%;
- &.is-button {
- border-radius: $radius-large;
+ .edit-site-style-book__liner {
+ flex: 1;
+ display: flex;
+ flex-direction: column;
}
- display: flex;
- flex-direction: column;
- align-items: stretch;
+ .edit-site-editor-canvas-container__close-button {
+ position: absolute;
+ z-index: 2;
+ }
}
.edit-site-style-book__iframe {
@@ -23,7 +25,6 @@
.edit-site-style-book__tablist-container {
flex: none;
-
display: flex;
width: 100%;
padding-right: 56px;
@@ -32,6 +33,5 @@
.edit-site-style-book__tabpanel {
flex: 1 0 auto;
-
overflow: auto;
}
|
Is this close button necessary? The revisions button already behaves like a toggle, and the There's some higher-level design work around Styles happening in #66376 that we should probably keep an eye on too. |
|
Hi @stokesman 👋. Thanks for helping out. I have added a couple of commits to add a |
Yeah, I'm not sure the close button is necessary. I also agree about the confusion when both Style Book and Revisions are open. I’m not sure removing or hiding the close button in Revisions would improve that yet it would fix its overlap with the scrollbar. From what I can see it wouldn’t interfere with the efforts in #66376 and just as a quick fix I think it’d be worth doing.
Thanks! That’s a move in the right direction code-wise. Though, at this point it does seem like it’s simpler to not show the close button for Revisions. I think that can be as simple as removing the |
|
@afercia, I thought you might be interested in weighing in on this. It seems like the two options for a straightforward fix are:
It might be worth keeping in mind that either way Revisions can also be closed by pressing the escape key while focus is in the canvas. |
afercia
added
[Type] Bug
|
@stokesman thanks for the ping. I was wondering as well whether the close button is necessary. Besides the scrollbar issue, there's more to consider:
Quoting from the issue:
Personally, between the two proposed options I'd opt for always having a header. However, the header should not be empty. It should contain a title: 'Style Book' and 'Revisions'. The Style Book tabs should be moved down to a second row. |
|
Hi @afercia 👋
Let me know your thoughts on it. Thanks |
|
@Vrishabhsk thank you. I'd totally support the new header and title. They solve a layout and an accessibility issue. To me, all panels in the editor should have a visible title. The I'd suggest to make the title a H2 heading but before any code change I suggest waiting for feedback from design. |
I second this. One thing that may come up is the header duplicates the title in the Document Bar. Also while it’s a bit of a separate concern, when both the Style Book and Revisions are open it may increase the confusion about what the "close" button is going to do. The header says "Style Revisions" and the close button is generic "close" label makes it visually seem like it will close revisions but it will only close the Style Book. 
The label for the Style Book’s close button could maybe be made more specific to help some and/or the title in the canvas container’s header made distinct when both are open—something like "Style Revisions / Style Book". Though that latter idea seems crummy to me compared to some better design solution which I haven’t been able to imagine yet. |
Yes, but I would argue that's a more general problem with the Document Bat that already happens for other cases. For example, the Post title is repeated 3 times in the UI. See point 3 of this issue description: #65238 |
What?
Close ButtonforGlobal Styles Revisionsview to prevent thescrollbarfrom overlapping on thebuttonWhy?
How?
positionasrelativefor theClose Buttonwhen viewing revisionsspacingandalignmentstyles to match that of theStyle Book Close ButtonviewTesting Instructions
Screenshots or screencast