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

adding button wrapper for icon #970

Merged
merged 2 commits into from
Oct 3, 2019

Conversation

michalica
Copy link
Contributor

@michalica michalica commented Sep 30, 2019

I want to improve accessbility of expandable rows in #969

@coveralls
Copy link

coveralls commented Sep 30, 2019

Coverage Status

Coverage remained the same at 76.634% when pulling bbd17e4 on michalica:adding-button-wrapper into f0ea9e5 on gregnb:master.

@michalica
Copy link
Contributor Author

Can you please include this PR in next release? @gabrielliwerant

@gabrielliwerant gabrielliwerant added the needs review Useful to mark PRs for what's up next to review label Oct 1, 2019
Copy link
Collaborator

@gabrielliwerant gabrielliwerant left a comment

Choose a reason for hiding this comment

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

Always nice to improve accessibility, thanks for looking into this!

We should use IconButton instead of button here, as that keeps us in line with material standards and the look of the icon that we had before.

There's also an opportunity here to add a onKeyDown handler for activation via the enter key. If you wanted to add that, I would be happy to accept it, or I can provide that functionality separately myself, it's up to you.

@gabrielliwerant gabrielliwerant added needs work and removed needs review Useful to mark PRs for what's up next to review labels Oct 1, 2019
@michalica
Copy link
Contributor Author

I would be glad to work on this. I think pressing space or enter on focused element triggers onClick event by default. In our case, row will be expanded or collapsed. Do we need onKeyDown handler there?

@gabrielliwerant
Copy link
Collaborator

You are correct, it looks like the default is to trigger on space and enter.

Ok, so let's just switch it to IconButton and we should also disable it for the header or we'll have an odd looking empty header button as well. Something like disabled={isHeaderCell} should work.

Copy link
Collaborator

@gabrielliwerant gabrielliwerant left a comment

Choose a reason for hiding this comment

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

Beautiful! Thank you for the update.

@gabrielliwerant gabrielliwerant merged commit 506a1d7 into gregnb:master Oct 3, 2019
waqasajaz pushed a commit to waqasajaz/mui-datatables that referenced this pull request Oct 31, 2019
* adding button wrapper for icon

* changing button ot IconButton, disable button form HeaderCell
lalong13 pushed a commit to lalong13/mui-datatables that referenced this pull request Jan 15, 2020
* adding button wrapper for icon

* changing button ot IconButton, disable button form HeaderCell
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.

3 participants