Skip to content
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

Use <main> element to match role=main area of body content #117

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Use <main> element to match role=main area of body content #117

merged 1 commit into from
Nov 19, 2021

Conversation

vsalvino
Copy link
Contributor

@vsalvino vsalvino commented Nov 18, 2021

See discussion in #113 . What do you think of this instead @Scotchester ?

Tested both before and after this change in various browsers and using the Windows narrator. Does not seem to be any visual or audible change.

I think use of these new HTML 5 elements seems to be splitting hairs in some places (i.e. difference between a <main> and an <article> element). However from researching MDN this current structure seems to be perfectly "legal" and make sense. Main is used for the actual page content (if you were to have a "skip to main content" button, it would jump here). Main can also contain headings, sections, paragraphs, etc.

Following this logic... sphinx looks for role="main" on page elements to determine what to index in its JavaScript-based search. So I don't think we'd want the breadcrumbs and "Edit on GitHub" buttons being indexed.

@vsalvino vsalvino mentioned this pull request Nov 18, 2021
Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

@vsalvino
Copy link
Contributor Author

Ok, I'll publish then. Feel free to merge unless anyone objects.

@vsalvino vsalvino marked this pull request as ready for review November 19, 2021 19:38
@Scotchester Scotchester merged commit 6eb67fb into wagtail:main Nov 19, 2021
@vsalvino vsalvino deleted the role-main branch November 22, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants