Use semantic element for block directory download heading#22713
Merged
Conversation
aduth
added
[Type] Bug
|
Size Change: -5 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
ryelle
approved these changes
May 28, 2020
Contributor
ryelle
left a comment
ryelle
left a comment
There was a problem hiding this comment.
h2 does appear to be the right fit here, since there is no other header in the inserter (and it follows the BlockCard component, which also uses h2). Everything looks good with the style changes too 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Slack conversation (link requires registration): https://wordpress.slack.com/archives/C02QB2JS7/p1590677131416000
This pull request seeks to improve the markup and styling associated with the block directory
DownloadableBlockHeadercomponent.While AXE tests are not anticipated to apply for block directory tests currently, due to an unrelated issue (#22712), they are, and the markup fails with a legitimate error about the markup being generated: "Required ARIA attribute not present: aria-level" (see related documentation).
Technically,
aria-levelcan be considered optional ("If no level is present, a value of 2 is the default"), though the AXE tests are configured to require an explicit value, likely to prevent implicit values from oversights of not considering to add the attribute. It's not clear if the original implementation omittingaria-levelwas an oversight or intentional (#17431).Ultimately though, it's not clear why the
roleis used here in the first place, vs. the more semantic heading elements. It may have been used merely as a convenience for styling, though it's not a very compelling reason, and the styling appears to seek to inherit some default heading styles anyways.For this reason, the component has been changed to render
<h2>, which should be semantically equivalent to how it was rendered previously.Styling has been updated for a few reasons:
paddingon the container, rather than multiplemarginon its children.font-weightheading styles, now inherited automatically ash2element.marginandcoloroverrides to the heading.Open Question:
Depending if it was intentional to omit
aria-leveland fall back to the default2, it would be good to clarify ifh2is the best fit here.Testing Instructions:
Verify that the presentation of the downloadable block header is identical in this branch compared to
master: