Allow accessibility attributes on the root element #114
Conversation
|
Hi there, thanks for the PR. I think a more straightforward and flexible strategy here is to forward all the props (less the |
| { backdrop && this.renderBackdrop() } | ||
| { dialog } | ||
| </div> | ||
| {React.createElement('div', |
There was a problem hiding this comment.
Can you change this back to jsx?
| @@ -267,20 +275,27 @@ const Modal = React.createClass({ | |||
| ); | |||
| } | |||
|
|
|||
| const modalProps = Object.assign( | |||
There was a problem hiding this comment.
instead of the whitelist you can do something like elementProps = _.omit(this.props, Object.keys(Modal.PropTypes))
| expect(attr).to.equal('modal-description'); | ||
| }); | ||
|
|
||
| it('Should accept the `tabIndex` property on the Modal', function () { |
There was a problem hiding this comment.
I don't think it should accept a tab index here, that would screw up the focus management. We already ensure that the dialog component has a tabIndex for a11y
|
Hmm there are a bunch of inconsistencies with how we manage roles here that I hadn't noticed before, in connection with the react-bootstrap modal. I need to think a bit on what might be the best way to move forward to fix them tho (pre-existing issues to your PR here) |
|
@jquense thanks for being so quick to review this PR. I've made your suggested changes, and I also took a stab at fixing some of the tests. Let me know of any other feedback. |
adamschroder
force-pushed
the
master
branch
from
d2080ec to
33a5c28
Compare
|
Not sure why the tests are failing on Travis, they are passing locally. Any ideas? |
| const propTypes = Object.keys(Modal.propTypes); | ||
| const newProps = {}; | ||
| keys.map(function (prop) { | ||
| if (!propTypes.includes(prop)) { |
There was a problem hiding this comment.
Array.prototype.includes is not available in all browsers. That's why tests are failing. Separately this should be an arrow function though. Even more separately this should just be what @jquense said with _.omit.
There was a problem hiding this comment.
Hey @taion, great catch. I didn't use _.omit because lodash is just a dev dependency in this project, and I didn't want to add it just for this. If ya'll are ok with adding it, i'll happily change the code.
|
@jquense Are your concerns regarding the pre-existing issues blockers for this PR? |
|
Hey guys, whats the status on this PR? |
|
hey @jquense, it has been a little over 2 weeks since there was any feedback here, any updates? |
| if (propTypes.indexOf(prop) === -1) { | ||
| newProps[prop] = _props[prop]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
can you hoist this out of hte render method as an omitProps function just so its a bit clearer what's going on?
|
i guess this is ok as is for now, could you also rebase the branch against master so it's a bit cleaner thanks! |
|
Updated & rebased. Thanks for the attention and feedback! |
| @@ -211,6 +211,19 @@ const Modal = React.createClass({ | |||
| }; | |||
| }, | |||
|
|
|||
| omitProps(props, desiredProps) { | |||
There was a problem hiding this comment.
Just pass in the propTypes object here, and check Object.prototype.hasOwnProperty.call
|
ping @jquense |
|
Hey guys, sorry for the spam, but can we try to get this merged? |
Hi there! I found some issues with this component in terms of accessibility. Particularly in how the Windows Narrator announces the Modal. From the spec , the
aria-labelledbyoraria-labelshould be an attribute on the same element as therole=dialog.Additionally, it would fit more use cases if the dialog could also be an alertdialog (or anything else that could also make sense, semantically).
If there are styling, formatting, or any other issues with this PR please let me know & i'd be happy to make them.