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

Add missing <main> element to documentation #22364

Merged
merged 15 commits into from
May 5, 2017

Conversation

Lausselloic
Copy link
Contributor

For accessibility improvement and beacause HTML5, I propose to add a main tag for the global docs content

@@ -12,9 +12,9 @@

{% include nav-home.html %}

<div id="content">
<main id="content" role="main">
Copy link
Contributor

@wolfy1339 wolfy1339 Apr 6, 2017

Choose a reason for hiding this comment

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

role="main" is not required here since we are using the <main> element

Copy link
Contributor Author

@Lausselloic Lausselloic Apr 6, 2017

Choose a reason for hiding this comment

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

I always double the main element with the aria role for IE11 who doesn't take care about this element

Copy link
Member

Choose a reason for hiding this comment

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

agree, it's unnecessary normally, but notoriously IE11 needs this extra role to make it work (as in, expose it correctly to AT). as it doesn't do any harm, this PR looks fine to me as is

Copy link
Member

Choose a reason for hiding this comment

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

Just a question about that but it seems main tag isn't supported by IE10 (https://caniuse.com/#feat=html5semantic).
What's the lower release of IE we support in our documentation ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes main tag doesn't fit well on IE10, need to add a css

main { display:block; }

Bootstrap compatibility is IE1à and upper ( https://v4-alpha.getbootstrap.com/getting-started/browsers-devices/#desktop-browsers)
So maybe need to add a fix in the core _reboot.scss for all HTML5 semantic default display?

article, aside, footer, header, hgroup, main, nav, section {
  display: block;
}

Copy link
Member

Choose a reason for hiding this comment

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

ah, in v3 we had html5shiv take care of that, forgot we don't have that in v4 anymore.

i think it'd make sense to add the default block styling to reboot. i'll take care of that before merging this, assuming @mdo is ok with it

Copy link
Member

Choose a reason for hiding this comment

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

done #22573

@@ -19,7 +19,7 @@
</div>
</div>

<div class="container">
<main class="container" role="main">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here

@patrickhlauke
Copy link
Member

Would suggest "Squash and merge" as there are quite a few extraneous commits in this PR

@@ -29,7 +29,7 @@ <h1 class="bd-title" id="content">{{ page.title }}</h1>
{{ content }}
Copy link
Member

Choose a reason for hiding this comment

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

wondering if this may not be the more appropriate location for <main> (depends if we count the side navigation as content or not). if so, i'd suggest keeping the <div class="col-12..."> and wrapping its contents in a <main role="main">, rather than replacing the <div> altogether

Copy link
Member

Choose a reason for hiding this comment

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

@Lausselloic you ok to make those change to this PR? need a hand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry haven't seen your comment. Yes I will do it

Copy link
Member

Choose a reason for hiding this comment

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

great stuff. will double-check it all works and merge. thanks :)

@patrickhlauke patrickhlauke changed the title docs - accessibility - add a main tag for content Add missing <main> element to documentation May 5, 2017
@patrickhlauke patrickhlauke merged commit c81be77 into twbs:v4-dev May 5, 2017
@patrickhlauke patrickhlauke added this to the v4.0.0-beta milestone May 5, 2017
@mdo mdo mentioned this pull request May 5, 2017
@Lausselloic Lausselloic deleted the v4-dev-a11y-docsimprovement branch May 5, 2017 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants