Skip to content

[added] accessibility props for PanelGroup and Panels. #1191

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

Merged
merged 2 commits into from
Aug 25, 2015

Conversation

kennyw12
Copy link

Adds accessibility, following the guideline specified here.

The Pivotal UI Team
@atomanyih @ctaymor @d-reinhold @kennyw12 @matt-royal @nicw @stubbornella

@kennyw12
Copy link
Author

Oops. Forgot PropType validation. I'm also not sure if these roles should be exposed.

@@ -166,7 +174,7 @@ const Panel = React.createClass({
if (this.props.collapsible) {
header = cloneElement(header, {
className,
children: this.renderAnchor(header.props.children)
children: this.renderAnchor(header.props.children, headerRole)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be incorrect for the header role to actually get attached to the "heading" element at https://github.com/react-bootstrap/react-bootstrap/pull/1191/files#diff-1e26d9c69579835d9b0137815187fafeR185 rather than the anchor?

Copy link
Member

Choose a reason for hiding this comment

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

The use-case we're anticipating here is using an accordion like a set of tabs, as in the example in the react-bootstrap documentation. Based on our research, the a is the correct place to put the role attribute. Here's one blog post that confirms this.

@taion
Copy link
Member

taion commented Aug 20, 2015

BTW, would appreciate seeing your thoughts on discussion on #983 from #983 (comment) on down, i.e. we might structure Panel to look like e.g. Modal, and have e.g.

<Panel>
  <Panel.Heading>{heading}</Panel.Heading>
  <Panel.Body>{body}</Panel.Body>
</Panel>

and then this would just work with the role prop set respectively on heading/body/whatever... haven't worked out all the kinks there yet though.

Signed-off-by: Dominick Reinhold <[email protected]>
Signed-off-by: Matt Royal <[email protected]>
@ctaymor ctaymor force-pushed the panels-accessibility branch from 20bf427 to adad32e Compare August 21, 2015 17:47
@ctaymor
Copy link
Member

ctaymor commented Aug 21, 2015

@mattroyal and I just amended the last commit to add role="presentation" to the h4 when we generate the header for a collapsible panel which was only passed a string for a header. The heading sizes are supposed to semantically describe the outline of the page, and since we don't know the structure of a user's page, we don't know what level the header should actually be. It is a visual aesthetic use of an h4, not a semantic one, thus presentation.

Edited to add: Just looking at #983 now and seeing that it addresses this issue. I left some thoughts on changing the API.

@taion
Copy link
Member

taion commented Aug 21, 2015

LGTM. Might make sense to squash these commits. Otherwise :shipit:!

@jquense
Copy link
Member

jquense commented Aug 25, 2015

LGTM

jquense added a commit that referenced this pull request Aug 25, 2015
[added] accessibility props for PanelGroup and Panels.
@jquense jquense merged commit b513d60 into react-bootstrap:v0.25-rc Aug 25, 2015
@atomanyih atomanyih deleted the panels-accessibility branch August 25, 2015 16:57
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.

None yet

5 participants