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

Add Sortable table example #2046

Merged
merged 57 commits into from
Oct 31, 2021
Merged

Add Sortable table example #2046

merged 57 commits into from
Oct 31, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Sep 27, 2021

I needed an example of a sortable table for someone at Illinois and thought I would propose it as an example for the APG.

Preview Link


Preview | Diff

@jongund jongund requested a review from jnurthen September 28, 2021 19:42
@mcking65
Copy link
Contributor

As a screen reader user, I find the button descriptions make the state of the table harder to understand. You have to listen very closely or you might think that the columns are are already sorted in the way the button description describes. I am not convinced we need descriptions. I wonder what other screen reader users think?

If there were descriptions, I'd rather have it be clearly distinguishable from sort state, perhaps "Sorts by COLUMN_NAME" and leave out the directionality of the action. The sort direction is conveyed after activation.

@jongund
Copy link
Contributor Author

jongund commented Oct 10, 2021

@mcking65
I changed the labels, let me know what you think.
The trick is not changing the accessible name of the button, otherwise the button accessible name gets used as the column name when using table navigation commands. But I am open to other ideas, But this is something that is very common and we should have some guidance in the practices.

@howard-e
Copy link
Contributor

Code review completed and LGTM! Have you been able to take a look at Matt's other requested changes @jongund?

@jongund jongund requested a review from mcking65 October 29, 2021 18:31
@jongund
Copy link
Contributor Author

jongund commented Oct 29, 2021

@howard-e I have addressed @mcking65 comments

@mcking65 mcking65 changed the title Sortable table example Add Sortable table example Oct 31, 2021
Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

I think we are very close to ready to ship. couple of questions left.

<h2>Accessibility Features</h2>
<ul>
<li>
HTML table markup is used to create the rows, caption, and to identify the column headers for the data cells. The use of HTML supports the ARIA principle of using native element semantics whenever possible.<br/>
Copy link
Contributor

Choose a reason for hiding this comment

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

          HTML table markup is used to create the rows, caption, and to identify the column headers for the data cells.  The use of HTML supports the ARIA principle of using native element semantics whenever possible.<br/>

I don't think we need to say this at all. As we add more and more HTML examples, saying we are using more HTML and less ARIA will be increasingly common and saying this over and over won't add value. Would you be OK with removing thisitem from the list?

<tr data-test-id="span-aria-hidden">
<td></td>
<th scope="row"><code>aria-hidden="true"</code></th>
<td><code>svg</code></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be span instead of svg?

@jongund
Copy link
Contributor Author

jongund commented Oct 31, 2021

@mcking65 I made update to the example option description based on your feedback, removed the section on using HTML table markup and changes svg to span for the aria-hidden entry in the roles, properties and states table.

@jongund
Copy link
Contributor Author

jongund commented Oct 31, 2021

@nschonni HTML linting problem with files unrelated to this pull request. Not sure how to "fix" this issues, should probably be a separate pull request and not fixed in this one.

@nschonni
Copy link
Contributor

Yes, it looks like the issues are related to #2055 landing, not this one

@nschonni
Copy link
Contributor

@jongund the linting issue is now fixed and green here again

@mcking65 mcking65 merged commit 9352378 into main Oct 31, 2021
@mcking65 mcking65 deleted the sortable-table branch October 31, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants