-
Notifications
You must be signed in to change notification settings - Fork 405
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
Accessibility enhancements #752
Conversation
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.
@xaksis tbh people shouldn't be using IE anymore. And you could choose to bump to major version, when dropping IE support. That way projects that choose to keep supporting IE can lock the version. And we can move on. |
Thanks for taking a look at this.
No, I have not — I don’t normally test for IE any more, and am not particularly well-set up for it. I’ll see whether I can find a relatively easy way to do it.
pw
… On Oct 18, 2020, at 7:00 AM, xaksis ***@***.***> wrote:
@xaksis commented on this pull request.
hey @plweil have you tested these changes on IE? A while back we had a lot of issues with wrapping rows/tds in templates because IE wouldn't render them.
here's the relevant issue #138
I would like to drop support for IE in the near future but for now there are still a lot of people using it there.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
To support IE or not? Your call. If you’d like me to test this pr in IE, I’ll try to do that. -pw
… On Oct 18, 2020, at 7:10 AM, p0ps ***@***.***> wrote:
@xaksis tbh people shouldn't be using IE anymore. And you could choose to bump to major version, when dropping IE support. That way projects that choose to keep supporting IE can lock the version. And we can move on.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@p0psicles you're right. Dropping IE support is the logical thing to do for the next major version. @plweil I very much want most of these changes in the current build. From what I can tell, the only thing that'd cause problems is the changes where you wrap thead contents in templates. Is it possible to create a PR without that change? We can save that one for when we bump up the major. |
src/components/Table.vue
Outdated
@@ -231,14 +239,13 @@ | |||
:formattedRow="formattedRow(row)" | |||
:index="index" | |||
> | |||
<span v-if="!column.html"> | |||
<template v-if="!column.html"> |
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 bit here is what I'm talking about.
@xaksis Thanks. I’ll look into this when I have a free moment.
… On Oct 20, 2020, at 9:59 AM, xaksis ***@***.***> wrote:
@p0psicles you're right. Dropping IE support is the logical thing to do for the next major version. @plweil I very much want most of these changes in the current build. From what I can tell, the only thing that'd cause problems is the changes where you wrap td contents in custom components/templates. Is it possible to create a PR without that change? We can save that one for when we bump up the major.
Also, if you could remove the built files from the PR that'd be great - it'll always cause conflict otherwise.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@xaksis I'm just getting back to this. Sorry for the delayed response. I'm aware of problems with IE and the element, but I always thought that these occur when is used outside the section of a component. But no matter. Removing the wrappers in the lines you cite should be fine. It'll just wrap those cell contents in an extra , which is not optimal but no big deal. I'll make that change and remove the built files from the PR shortly. |
@xaksis do you need anything else from me to merge this, or is this ok? I did remove the built files. |
hey @plweil sorry, the PR seems to still include built files and is showing me a bunch of conflicts there so wasn't sure if you were done. Do you mind taking a look maybe it wasn't committed? |
Will do.
… On Dec 7, 2020, at 4:15 PM, xaksis ***@***.***> wrote:
hey @plweil sorry, the PR seems to still include built files and is showing me a bunch of conflicts there so wasn't sure if you were done. Do you mind taking a look maybe it wasn't committed?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Sorry if I'm being dense about this, but I thought I deleted the built files in commit 067cabe. What am I doing wrong? |
You shouldn't delete them. You should copy the files from master into your branch. That way there is no change to them. |
Oh jeez. Thank you @p0psicles. I've re-added the built files from master. |
@xaksis All good now? |
@plweil done! Thank you for your contribution and a very Happy New Year! |
@xaksis good deal and thank you! Happy New Year to you too! |
@xaksis I'm working on a followup a11y PR -- do I still need to account for IE support? |
addresses #751
Adds the following accessibility enhancements:
Table header sort buttons. Can be reached via the keyboard. Dynamically labeled "Sort table by [column] in [ascending/descending] order." Buttons are absolutely positioned to cover the entire
th
box. The pseudo-elements for the "sort arrows" are placed on thebutton
element instead of theth
.aria-sort
attribute to inform user of the current sort order. E.g.,aria-sort="ascending"
.th
elements are given an explicit role ofcolumnheader
to accommodatearia-sort
."sort arrows" via pseudo-elements w/chevron backgrounds left intact, except
border
color is darkened to improve color contrast.Table search: add
label
element to provide accessible name, which is visually-hidden but accessible to screen readers via newsr-only
css class. Magnifying glass is darkened for color contrast.placeholder
attribute is set to null by default (placeholders might look good to some, but they are an inadequate substitute for labels, and can cause problems for a range of users.).The bottom footer area is changed as little as possible while increasing the font size ; darkening the color; removing Vuetify css for the
select
element; adding alabel
element; separating the pagination info from the Previous and Next page buttons for improved clarity; changing the latter to button elements; change the default button label from 'Prev' to 'Previous'; add some margins, etc. to clean up appearance. Button focus outlining is restored; outlines are important for AT users and should not be removed.no changes were made to 'pages' pagination mode. I can't help but think that a solution with page number buttons instead of manually entering a page number would be better. But I don't have enough time to work on that right now.
My chief concern here is accessibility and UI clarity. I don't have a strong opinion regarding the darker colors added here; these colors at least provide an example of better contrast. See, for example the WET project.