-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Pr/2395 updated #2584
Pr/2395 updated #2584
Conversation
@jquense Should |
I didn't change it because the upstream docs use |
the upstream docs use an i thought there was something with the |
The usage is slightly different tho, since in this case one is a Button and the other is jsut a div. I think the problem is due to JAWS handling of button text, not just it's reading of aria-label (which is does generally) |
what do you mean? it's a |
what aria-label are we talking about? I was looking at the Modal.Title, do you jsut mean the close button there? |
wait... |
wtf |
nvm I'm confusing two things. yes we should make this consistent with the alert close btn |
src/ModalHeader.js
Outdated
onClick={createChainedFunction(modal && modal.onHide, onHide)} | ||
> | ||
<span className="sr-only">{closeLabel}</span> |
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.
why are these in a different order than on the alert? :p
also do we want to fix the prop name here for good?
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.
order shouldn't matter but i can change it. I do want to fix the naming, i forgot this was going into next
👍
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.
better to be consistent than inconsistent between two parallel use cases :p
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.
better yet just make it a component
updated #2395