Heading block: use toolbar for heading level control #20246
Conversation
ZebulanStanphill
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
| minLevel={ 2 } | ||
| maxLevel={ 5 } | ||
| minLevel={ 1 } | ||
| maxLevel={ 7 } |
There was a problem hiding this comment.
No, this is not a mistake. The maxLevel is actually exclusive rather than inclusive, so you have to set it to 7 if you want to include heading level 6. I thought that was weird, but I decided it would be best to address it in a separate PR, if it needs to be addressed at all. Does anyone else think this is weird?
There was a problem hiding this comment.
The fact that heading 1 was omitted from the toolbar was a requirement/recommendation from the a11y team to discourage its usage as it's generally used for post title in the templates. Is this something we want to reconsider?
There was a problem hiding this comment.
@youknowriad Some kind of warning would be helpful, similar to the color contrast warning (which can be used in the toolbar, as demonstrated by the Navigation block). No equivalent currently exists for heading levels except in the Document Outline.
Showing a duplicate control in the inspector with the discouraged options is not the right answer, in my opinion. We should be dealing with incorrect heading levels the same way we deal with low-contrast background/text color combinations: by warning the user when they choose a potentially poor option.
|
Thanks for adjusting this feature Zeb! This is what it looks like right now: It looks like a bug. |
youknowriad
added
Needs Accessibility Feedback
|
The E2E test failure is because in I'm not sure how to update the test to click the button in the block toolbar, however. Something I have just discovered is that the heading level button in the toolbar doesn't have any I fixed the label issue in #20248. I guess that PR needs to be merged before this one so I can fix the E2E test. |
ZebulanStanphill
requested review from
ellatrix,
nerrad,
ntwb and
youknowriad
as code owners
|
Would it be possible to do something like this, were we don't need to repetitive "Heading" labels for each option?
|
|
I've temporarily included changes from #20248 to show that, with the label fix from that PR merged and with the e2e test adjusted, all tests pass. |
|
@shaunandrews I'm sure it would be possible, but it's out of the scope of this PR, and it might have negative effects on accessibility due to the removal of visible labels. I'd recommend opening a separate issue for that. |
|
Whilst I am for them all going in toolbar, I do wonder if what @shaunandrews is suggesting would be a better visual. Whilst I understand the focus of this PR iterating on the design to be the best possible one I think might work here, or at least we should rapidly get an issue up for that. Having such a long list in dropdown/menu feels very cumbersome. It's the problem with content and UI not work on this PR though. |
ZebulanStanphill
force-pushed
the
update/heading-block-level-controls
branch
2 times, most recently
from
d40edd6
to
8d63e11
Compare
|
#20248 has been merged into |
|
Size Change: +112 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
ZebulanStanphill
force-pushed
the
update/heading-block-level-controls
branch
from
8d63e11
to
a2340eb
Compare
|
Rebased; everything looks fine in the new G2 UI. |
|
@ZebulanStanphill thanks for working on this. What I currently see is:
I still personally feel having just the idea that @shaunandrews shared would be better. Would you be able to work on that as an iteration? |
|
@shaunandrews' mockup looks cool, but it seems like it might be less accessible than the existing dropdown. However, I'm willing to try it out. Unfortunately, I'm not sure how to go about implementing it. There are no existing implementations of anything quite like his mockup. Do I use I also find it confusing that the toolbar has more visible information for each option when |
|
Speaking of simplified dropdown menus, the newly-opened #20761 is quite relevant. |
ZebulanStanphill
force-pushed
the
update/heading-block-level-controls
branch
from
a2340eb
to
5158cb7
Compare
There was a problem hiding this comment.
Great work @ZebulanStanphill!
I tested it on Chrome, Safari and Firefox and can confirm that the issue with closing the popover when pressing any of the heading toolbar buttons happens on Safari and Firefox (on MacOS). So, it's probably related to buttons not receiving focus on click on those browsers. I just suggested a temporary workaround for that. But I'll try to find a better solution in the Toolbar module itself.
Also, arrow keys aren't working on the heading dropdown toggle button. And a simple solution for that is to just use ToolbarButton instead of ToolbarItem.
ZebulanStanphill
force-pushed
the
update/heading-block-level-controls
branch
3 times, most recently
from
4dfb0c6
to
a87fc5d
Compare
|
Thanks for the help, @diegohaz! I've switched to using a standard Everything appears to be working as intended now! 😄 |
There was a problem hiding this comment.
Tested it on Chrome, Firefox and Safari, and it looks great! Also, I was able to use it with VoiceOver + Safari and NVDA + Firefox without problems. Couldn't find any other issue.
The only thing that feels odd to me is that the initial focus isn't on the current selected item. But this is probably something out of the scope of this PR as the same happens to the text alignment option.
|
The Heading block also works now in Safari. Good job Zeb! |
ZebulanStanphill
force-pushed
the
update/heading-block-level-controls
branch
from
a87fc5d
to
16e582e
Compare
|
One thing though. |
Fix lint errors. Change the native side to use a dropDown menu directly.
ZebulanStanphill
force-pushed
the
update/heading-block-level-controls
branch
from
16e582e
to
e0b2a1b
Compare
I tried it out myself, and the behavior appears totally normal to me. Each heading size is a different height, so of course the appender would move up/down. The block is changing height, so the appender moves to stay below the block. Or are you talking about something else? |
|
Hey Zeb. Yes, I am talking about the moving inserter like so:
But I do understand what your saying. As the size of the block changes the location of the inserter changes. Making it jump up or down based on which heading size is selected. |
ZebulanStanphill
removed
Needs Accessibility Feedback
|
Now that this PR is out of the way, I've started working on adding a heading level checker to the block to encourage more semantic heading level choices: #22650. |
oandregal
changed the title
Description
Having the heading level picker in both the toolbar and inspector is confusing and inconsistent. It's also weird that only the inspector version contains the h1, h5, and h6 options.
This PR removes the inspector control and puts all the heading level options into the toolbar dropdown.
How has this been tested?
I opened up a post and played around with the heading level toolbar control to verify that it worked.
Screenshots
UPDATE: based on feedback, I've updated the control to look like this:
Checklist: