-
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
DocumentBar: replace icon with post type label #65170
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. |
aria-label={ | ||
! props.title && TYPE_LABELS[ postType ] | ||
? // eslint-disable-next-line @wordpress/valid-sprintf | ||
sprintf( TYPE_LABELS[ postType ], title ) | ||
: undefined | ||
} | ||
> |
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 aria-label doesn't seem necessary any more, since the post type is now displayed after the document title.
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.
I noticed that this removal causes some of the e2e tests to fail. E.g. this one:
test( 'Allow to switch to template mode, edit the template and check the result', async ( { |
The failure appears to be the check on this line:
gutenberg/test/e2e/specs/editor/various/post-editor-template-mode.spec.js
Lines 198 to 200 in 50f5c74
const title = this.editorTopBar.getByRole( 'heading', { | |
name: 'Editing template: Single Entries', | |
} ); |
The aria-label does add a bit more context as it explains the context (editing) rather than using the visual separator. So I wonder if it's worth still including it? Either that, or updating the test 🙂
I'll just ping @jeryj on this one, too, as I remember there being previous discussions about the accessibility of this bar.
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.
I don't think this impacts it one way or another. The main issue with the accessibility of this area is the semantics and inconsistent naming: #51394.
- Buttons should have consistent descriptive names, so this button needs to be visually and consistently named as "Command Palette" or something similar.
- The
<h1>
is nested within the<button>
element.<button>
elements often strip the semantics of its contents, so the<h1>
would never be exposed. It is as if there is no heading on the page at all.
So, those core issues do need to be fixed, but this PR doesn't seem to impact the core issues either way.
Size Change: +986 B (+0.06%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
This is looking really nice to me in testing! Just pushed a tiny update to try to fix one of the e2es. Looks like there's another that will need fixing (or for us to restore the aria-label): #65170 (comment) ✅ Looking good in the page editor, post editor, and site editor: ✅ Looking good in custom post type editor: ❓ One thing I noticed while testing is that the DocumentBar appears to be displayed in bold in RTL languages (otherwise RTL appears to be working pretty well): This appears to be because of rules output via the localization stylesheet: To resolve that, I think we could add gutenberg/packages/editor/src/components/document-bar/style.scss Lines 59 to 65 in 7ba8423
Does that sound worth doing in this PR? If not, happy to open a separate one-liner PR for it 🙂 |
@@ -66,7 +66,7 @@ | |||
} | |||
|
|||
.editor-document-bar__shortcut { | |||
color: $gray-800; | |||
color: $gray-700; |
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.
This may need to remain $gray-800, or be #6d6d6d to meet 4.5:1 contrast. https://contrast.tools is a great way to test.
Other than that, ship it!
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.
I note that with long titles the type label will be truncated:
Maybe it’d be preferable to keep the type label visible:
Diff to have title truncated separately from post type
diff --git a/packages/editor/src/components/document-bar/index.js b/packages/editor/src/components/document-bar/index.js
index be291d6be5..84b12a7d42 100644
--- a/packages/editor/src/components/document-bar/index.js
+++ b/packages/editor/src/components/document-bar/index.js
@@ -181,12 +181,14 @@ export default function DocumentBar( props ) {
{ title
? decodeEntities( title )
: __( 'No title' ) }
-
- { postTypeLabel && ! props.title && (
- <span className="editor-document-bar__post-type-label">
- { ' · ' + decodeEntities( postTypeLabel ) }
- </span>
- ) }
+ </Text>
+ <Text
+ size="body"
+ className="editor-document-bar__post-type-label"
+ >
+ { postTypeLabel &&
+ ! props.title &&
+ ' · ' + decodeEntities( postTypeLabel ) }
</Text>
</motion.div>
<span className="editor-document-bar__shortcut">
Note that doing so would mean responsive styling will need some attention:
Nice work @creativecoder! I've pushed a couple of tiny commits to get the e2e tests passing since we're agreed on removing the I did a bit more hacking, and I think I've come up with a way to improve the responsive behaviour for now, if we're willing to accept hiding the post type label when the Back button is present, at an in-between viewport size. It's a little awkward adding the media query, but seems to work okay if we also move the diff --git a/packages/editor/src/components/document-bar/style.scss b/packages/editor/src/components/document-bar/style.scss
index bd85ab0db05..2308554d473 100644
--- a/packages/editor/src/components/document-bar/style.scss
+++ b/packages/editor/src/components/document-bar/style.scss
@@ -25,6 +25,14 @@
background: $gray-200;
}
}
+
+ &.has-back-button {
+ @media screen and (min-width: #{ ($break-medium) }) and (max-width: $break-large) {
+ .editor-document-bar__post-type-label {
+ display: none;
+ }
+ }
+ }
}
.editor-document-bar__command {
@@ -37,6 +45,7 @@
overflow: hidden;
color: $gray-900;
margin: 0 auto;
+ max-width: 70%;
// Offset the layout based on the width of the ⌘K label. This ensures the title is centrally aligned.
@include break-medium() {
@@ -55,7 +64,7 @@
.editor-document-bar__post-title {
color: currentColor;
- max-width: 64%;
+ flex: 2;
overflow: hidden;
text-overflow: ellipsis;
@@ -65,6 +74,7 @@
}
.editor-document-bar__post-type-label {
+ flex: 0;
color: $gray-800;
padding-left: $grid-unit-05;
} The result would be this: 2024-09-12.15.36.06.mp4(Happy to commit to this branch, but I didn't want to make any changes in case there were particular decisions you'd wanted to hold onto in the current shape of the CSS 🙂) |
@andrewserong That approach for responsive styles seems like a reasonable compromise. Please feel free to commit to this branch! I appreciate the help--this has already taken a lot more time than I was expecting. |
@jameskoster I'm open to it, but while trying it out, it seems redundant for the themes I've tested. A couple examples: compared to using the post type Not a big sample size, but the block themes I see put "Header" or "Footer" in the names of those template parts. For other template part areas, I'm not sure that the "General" label adds useful information compared to just "Template Part" (e.g. "Sidebar · General Area" vs "Sidebar · Template Part") Do you have any other suggestions for the wording and format of the template part area label that would make it more informative? |
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.
Thanks @creativecoder! I've pushed the fix for that in-between breakpoint and using flex
rules where appropriate. After re-testing this is working nicely for me now, I've tested a bunch of different screens in Chrome, FF, Edge, and Safari, and it's holding up nicely for me.
I'll give it the tentative approval, but might be good to get a +1 from designers just to make sure it's looking good to land as an iteration. And we can always keep tweaking in follow-ups.
(Also, I noticed a couple if intermittent e2e test failures, but I'm pretty sure they're unrelated to this PR, 🤞 they pass from running them again)
LGTM! 🎉
Re: failing e2e tests — I believe it's due to a change in |
Not off the top of my head. I agree that |
bf8325c
to
b01b483
Compare
Alrighty, I've rebased this PR and the tests are passing now. @WordPress/gutenberg-design any objections if we land this PR in its current form, or were there any design tweaks you'd like changed before merge? |
@andrewserong There's one more thing with the responsive styles I'd like to fix: the post type shows instead of the title at very small screen widths: I think this should be justified such that the post type clips rather than the title (or maybe the post type just doesn't show until there's enough room for it.) Other than that, this is looking good to me. Will tackle issue tomorrow, unless you beat me too it. |
Oh, good catch! Maybe we can just hide the post type on very small screens, similar to what we've done with that in-between break point. I've had a go at that in 6f9ae9a, but feel free to revert if there's a better way to go about it (or of course land this if it's looking good 😄). Here's how it's looking to me now: 2024-09-16.14.49.33.mp4 |
I think this is in a good place now, and there haven't been any objections so far, so I'll merge this in. Happy to help on any follow-ups if need be! |
Thanks for finishing this one up @andrewserong , much appreciated! |
What?
Replaces the post type icon in the document bar with an explicit label
Resolves #65167
Why?
To help make it more obvious exactly what kind of document you are editing.
How?
Testing Instructions
Testing Instructions for Keyboard
n/a
Screenshots or screencast