-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
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
Styling and accessibility clarification for navs documentation #22472
Styling and accessibility clarification for navs documentation #22472
Conversation
@mdo for context here, requesting review purely as a quick read-through/check that what i say about the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been removing callouts wherever possible in v4. We got super deep on them in v3 and it was overwhelming. Instead, I want as many words in our docs to be essential as possible—callouts should be used rather sparingly.
The base `.nav` component is built with flexbox and provide a strong foundation for building all types of navigation components. It includes some style overrides (for working with lists), some link padding for larger hit areas, and basic disabled styling. | ||
|
||
{% callout info %} | ||
The base `.nav` component does not include any `.active` state. The following examples include the class, mainly to demonstrate that this particular class does not trigger any special styling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep this as the single sentence in the above paragraph. I don't think this really clarifies anything and instead draws attention away from what .nav
includes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think as this is a bit "meta" (in that it clarifies why all the following examples use .active
but don't look any different) it would warrant being in a callout. otherwise it can and will easily be missed (see #22470)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdo are you ok with me just leaving this one callout in (and neutering the others)?
docs/components/navs.md
Outdated
@@ -235,6 +239,10 @@ If you need responsive nav variations, consider using a series of [flexbox utili | |||
|
|||
If you're using navs to provide a navigation bar, be sure to add a `role="navigation"` to the most logical parent container of the `<ul>`, or wrap a `<nav>` element around the whole navigation. Do not add the role to the `<ul>` itself, as this would prevent it from being announced as an actual list by assistive technologies. | |||
|
|||
{% callout info %} | |||
Note that navigation bars, even if visually styled as tabs with the `.nav-tabs` class, should **not** be given `role="tablist"`, `role="tab"` or `role="tabpanel"` attributes. These are only appropriate for dynamic tabbed interfaces, as described in the [<abbr title="Web Accessibility Initiative">WAI</abbr> <abbr title="Accessible Rich Internet Applications">ARIA</abbr> Authoring Practices](https://www.w3.org/TR/wai-aria-practices/#tabpanel). See [JavaScript behavior for dynamic tabbed interfaces](#javascript-behavior-for-dynamic-tabbed-interfaces) in this section for an example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is under the section heading ## Regarding accessibility
—I don't think it needs to be a callout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thing, can make this regular para
docs/components/navs.md
Outdated
|
||
Use the tab JavaScript plugin—include it individually or through the compiled `bootstrap.js` file—to extend our navigational tabs and pills to create tabbable panes of local content, even via dropdown menus. | ||
|
||
{% callout info %} | ||
Dynamic tabbed interfaces, as described in the [<abbr title="Web Accessibility Initiative">WAI</abbr> <abbr title="Accessible Rich Internet Applications">ARIA</abbr> Authoring Practices](https://www.w3.org/TR/wai-aria-practices/#tabpanel), require `role="tablist"`, `role="tab"`, `role="tabpanel"`, and additional `aria-` attributes in order to convey their structure, functionality and current state to users of assistive technologies (such as screen readers). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove this as a callout and treat it as generic text, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, will make regular para
urgh, managed to completely mess up this PR with that rebase :( |
@mdo if you're ok with leaving just that first thing as a callout, as per my comment, i'm happy to close this PR and make a fresh clean one for merging... |
@mdo I may be bold and merge this with that one callout (which I think is quite easy to miss unless you're reading everything very carefully, and leaves you wondering why |
Closes #22470