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

Accessibility: Warn if multiple H1 headings used #4238

Merged
merged 2 commits into from Jan 5, 2018

Conversation

Projects
None yet
6 participants
@youknowriad
Contributor

youknowriad commented Jan 3, 2018

closes #4230

This PR updates the document outline to mark h1s as invalid if we use more than one h1s.

Testing instructions

  • Add two h1 headings to the post content
  • Open the document outline
  • Notice the headings are marked invalids with a message.

@youknowriad youknowriad self-assigned this Jan 3, 2018

@youknowriad youknowriad requested a review from afercia Jan 3, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jan 3, 2018

Contributor

So basically, when merged, it will look like this:

screen shot 2018-01-03 at 17 23 37

Couple issues:

  • we don't know how the post title is rendered in the frontend: it may or may not be a H1
  • space is limited and we'd need a better message than Multiple H1 headings

Would it be possible to:

  • when there's a post title and one H1, use a message: Your theme may already use a H1 for the post title
  • when there are multiple H1s, use a message: Multiple H1 headings are not recommended
  • consider that in other languages even the current strings will probably be longer than the one in English; when a heading is nested by multiple levels, there's really a very limited space (see below). Maybe consider to make the whole dropdown larger?

screen shot 2018-01-03 at 17 36 46

Note: the headings themselves could be way longer.

Contributor

afercia commented Jan 3, 2018

So basically, when merged, it will look like this:

screen shot 2018-01-03 at 17 23 37

Couple issues:

  • we don't know how the post title is rendered in the frontend: it may or may not be a H1
  • space is limited and we'd need a better message than Multiple H1 headings

Would it be possible to:

  • when there's a post title and one H1, use a message: Your theme may already use a H1 for the post title
  • when there are multiple H1s, use a message: Multiple H1 headings are not recommended
  • consider that in other languages even the current strings will probably be longer than the one in English; when a heading is nested by multiple levels, there's really a very limited space (see below). Maybe consider to make the whole dropdown larger?

screen shot 2018-01-03 at 17 36 46

Note: the headings themselves could be way longer.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 4, 2018

Contributor

Updated with the suggestions in #4238 (comment)

Contributor

youknowriad commented Jan 4, 2018

Updated with the suggestions in #4238 (comment)

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 4, 2018

Contributor

@afercia @youknowriad this will be much better once the title is just a block, but in the meantime we could also let themes inform us which heading level they use for post titles via:

add_theme_support( 'gutenberg', array(
   'wide-images' => true,
   'post-title-heading' => 'h1',
) );

If this is present, we use the provided value; if it's not, we use the current "title" label. What do you think?

Contributor

mtias commented Jan 4, 2018

@afercia @youknowriad this will be much better once the title is just a block, but in the meantime we could also let themes inform us which heading level they use for post titles via:

add_theme_support( 'gutenberg', array(
   'wide-images' => true,
   'post-title-heading' => 'h1',
) );

If this is present, we use the provided value; if it's not, we use the current "title" label. What do you think?

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jan 4, 2018

Contributor

@mtias could that help also future support for custom post types that don't use the post title? For example, with a value none. In that case, it could be also required.

Contributor

afercia commented Jan 4, 2018

@mtias could that help also future support for custom post types that don't use the post title? For example, with a value none. In that case, it could be also required.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 4, 2018

Contributor

@mtias What about themes with multiple templates and CPTs, some using h1, and some not. I think this doesn't bring enough value compared to the complexity to handle all the use-cases.

Contributor

youknowriad commented Jan 4, 2018

@mtias What about themes with multiple templates and CPTs, some using h1, and some not. I think this doesn't bring enough value compared to the complexity to handle all the use-cases.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 5, 2018

Contributor

Mmm, yes, probably not worth the headaches involved in all the possible variations at the template level.

Contributor

mtias commented Jan 5, 2018

Mmm, yes, probably not worth the headaches involved in all the possible variations at the template level.

@sharazghouri

This comment has been minimized.

Show comment
Hide comment
@sharazghouri

sharazghouri Jan 5, 2018

Contributor

good

Contributor

sharazghouri commented on 6bf3f89 Jan 5, 2018

good

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 5, 2018

Contributor

All concerns seems addressed, merging

Contributor

youknowriad commented Jan 5, 2018

All concerns seems addressed, merging

@youknowriad youknowriad merged commit 57e5a6b into master Jan 5, 2018

3 checks passed

codecov/project 40.61% (+1.19%) compared to 9fd2aa8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/h1-warning branch Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment