-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Post Title Block: add typography formatting options #31623
Conversation
packages/block-editor/src/components/text-decoration-control/index.js
Outdated
Show resolved
Hide resolved
Size Change: +157 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
Thanks for working on this @creativecoder Grant! I added the Post Title block to a page. Thank you for the additional controls! I went through and tested all of these. The controls works nicely! As I added the block to a page. The page end up with having two titles. The static title and the Post Title block title. |
Cool, thanks for your work here. This is before: This is after: I know I've noted this in a couple of adjacent tickets, but it's important to reiterate. These text-decoration and letter case properties work well but are somewhat exotic/advanced controls, meaning until we have a better syste, for progressively adding these (as outlined in #27331), we should only be adding them to exotic/advanced blocks. In this case, adding it to Post Title seems fine, as that is exactly that: an advanced block as part of the FSE effort. So that's just to say, we need to refine the user experience before adding it to something like Paragraph. |
Testing this out everything (except the below) seems to work well. Im not sure if this is an issue with the hook used or specific to its implementation here, but when I try to press the "A" button in text decoration it doesn't seem to change anything: Also (more of a critique of this tool in general), there is no tooltip when hovering these buttons. I can't actually remember what "A" is supposed to do! 😆 |
@Addison-Stavlo |
I had to inspect the DOM to find the Aria label that says "None" to figure out what the A meant, so the icon isn't super clear as "none". But digging a bit further, I'm now not sure where it's coming from. Looking at the Navigation block, which I believe is one of the first blocks to use the text-decoration control, it doesn't look like it has that "none" option: @aaronrobertshaw I think you worked on text decoration, do you recall where that's from? |
This isn't from the initial implementation of the text decoration block support. It was added as part of this PR we are looking at. My guess is that the Originally, it was to use a ButtonGroup ( #26059 (comment) ) including an option for |
Good assessment, and I think it could work if it shared more DNA with the underline and strikethrough icons, say had the same font size and throughline.
Thanks for the links, the segmented control still makes sense since for this property you can still only actually choose one at a time, not multiple. Thinking in terms of the user experience, the tricky bit here is to help indicate why the "none" option exists at all, when you can just untoggle one of the others. I.e. what's the difference between explicitly toggling "none" and not toggling any of the options? — I know the technical difference, but it's hard to convey. We might just need better icons here and things will fall in place. I can look at this when I get a moment. |
Exactly. The difference between an explicit none or "clear decoration" and the "default" or "inherited". It's a tricky one to convey via an icon. |
I like how Figma represents "none" with a dash: This makes the pattern consistent and easy to identify across different controls. Using a letter like the "A" is always going to be somewhat ambiguous since it can easily be confused for "upper case" or "lower case". I know these aren't text decoration properties but the average user probably does not, and the Letter Case control may not always be present to guide them. |
Thank you for all the feedback and sorry for the confusion on the "none" setting and the icon, let me try to clarify: I grabbed the The option to use I'll proceed with the following:
|
Now that I know that the regular "A" means "none"... this totally makes sense. 😆 🤦♀️
Great point! |
A text that is not a link should almost never be underlined, this is a big accessibility issue. |
I'll second @carolinan . My belief is most text formatting can get users in to trouble. Not so much for screen readers as much as it would effect people with low vision who rely on those different looking text clues. I think this whole PR could use a review from the Accessibility team as a whole and see if any other of these formatting concerns could come up aside from the underline. Thanks. |
Underlining unlinked text does not directly trigger an accessibility problem, but for sighted users underlining text that is not linked creates usability problems. Users will reflexively expect the underlined text to be a link. I wouldn't consider this an accessibility problem, per se, since it impacts all users who are able to perceive the underline pretty much equally. Whether we should offer this as an option or not is somewhat a general question of whether WordPress should encourage best practices. Underlined text that isn't linked is generally a poor practice from a design and usability standpoint. I generally think that WordPress should avoid offering this as an option. Regarding other options: Does the strikethrough toggle the semantic state of the text? If it wraps the text in All caps can also create accessibility problems, but since users have always been able to get around that by typing in all caps, I don't see any reason not to provide that option. |
Separate PR for adding the "none" option to the text decoration controls: #31768 |
A summary of the Accessibility teams discussion can be found here. https://make.wordpress.org/accessibility/2021/05/14/accessibility-team-meeting-notes-may-14-2021/ |
It looks like removing the underline was the recommendation for a11y, is that correct? |
The underline itself, when the post title is linked, should be the default text decoration. A formatting option should not make it possible to add underlines to the text when it is not a link. At some point in the future, I trust that there will be ways to handle text decorations and hover styles and colors for links. It is unclear if is a good idea to expose it as controls to users. |
In my mind, exposing underline controls to users for links or headings make sense primarily in the sense that they would empower you to "fix" a bad theme that you otherwise love: maybe the theme comes with no underlines to those links, and by surfacing the tools you can add them back. You could also have done that by choosing another theme, or by writing custom CSS. But the underline tool itself is agnostic, it can be used for good or evil. What we can do is push towards good defaults and practices. Using a progressive reveal system, we can hide underline controls by default, adding friction. We can even show contextual warnings or notices when you use the features in a bad way, e.g. if you employ an underline on a paragraph, we can output a big text warning — just like the contrast checker — explaining why that's a bad idea. That kind of education, paired with the tools, might ultimately be the better solution than hiding the controls. |
Yes, totally agree. This has been my experience so far. Without these controls, there's simply no way to get around theme default choices. How could we move this PR forward? From my perspective, the controls that would make the biggest impact design-wise are font weight and font style. If text decoration and transformation need more discussion, maybe we could break them out into a separate PR, and get the other enhancements shipped? |
Can we get all the other options except text-decoration merged and continue any approach there separately? |
@creativecoder are you wanting to continue with this PR or are you happy for someone else to make the change Matias suggests to get it over the line and merged? |
4de6370
to
890ef1d
Compare
@mtias @glendaviesnz Sorry for the delay, I lost track of this one. It's now updated and rebased, but omitting the text decoration controls. A note about testing, there seems to be an issue with using the "Make a title a link" setting in the Post Title block, I get This should be ready for review... there's not much to it, now! |
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.
Works well for me, and I also do not see a JS error for the link (Chrome, Windows 10). |
Good to know! Maybe it's just me. |
* trunk: (74 commits) Update docs for ClipboardButton component (#34711) Post Title Block: add typography formatting options (#31623) Bump plugin version to 11.5.0 Navigation Screen: Adjust header toolbar icon styles (#34833) Fix the parent menu item field in REST API responses (#34835) Rewrite FocusableIframe as hook API (#26753) Create Block: Remove wp-cli callout since not recommended and outdated (#34821) Global Styles: Fix dimensions panel default controls display (#34828) [RNMobile][Embed block] Enable embed preview for Instagram and Vimeo providers (#34563) Increase Link UI search results to 10 on Nav Editor screen (#34808) Prevent welcome guide overflow x scroll (#34713) Enable open on click for Page List inside Navigation. (#34675) [RNMobile] [Embed block] - Unavailable preview fallback bottom sheet title update (#34674) Add missing field _invalid in menu item REST API (#34670) Fix Dropdown/DropdownMenu toggle closing in all UAs (#31170) Navigation submenu block: replace global shortcut event handlers with local ones (#34812) Navigation Screen: Consolidate menu name and switcher (#34786) Remove parent and position validation from menu item REST API endpoint (#34672) Clean theme data when switching themes in the customizer (#34540) Components: add reset timeout to ColorPicker's copy functionality. (#34601) ...
As the Post Title Block has been taken care of it seems natural to help @vdwijngaert Koen with the PR on moving the Post/Page title to the top bar so that we can perhaps get it into WP 5.9. |
Description
Adds text formatting options to the Post Title block using experimental block supports declarations.
Summary of changes
Partially addresses #31117, but leaves out text decoration, for now
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.js
files for terms that need renaming or removal).