va-table: add sorting variation #1358

it-harrison · 2024-10-07T23:06:19Z

Chromatic

https://sortable-table--65a6e2ed2314f7b8f98609d8.chromatic.com

Description

This PR makes the va-table sortable by means of a sortable prop.
Key points:

the table can be sorted asc/desc by any column so long as the data in the column is numeric, date, or string
the data projected into each table slot must be a string, not a component

Closes 2771

QA Checklist

Component maintains 1:1 parity with design mocks
Text is consistent with what's been provided in the mocks
Component behaves as expected across breakpoints
Accessibility expert has signed off on code changes (if applicable. If not applicable provide reason why)
Designer has signed off on changes (if applicable. If not applicable provide reason why)
Tab order and focus state work as expected
Changes have been tested against screen readers (if applicable. If not applicable provide reason why)
New components are covered by e2e tests; updates to existing components are covered by existing test suite
Changes have been tested in vets-website using Verdaccio (if applicable. If not applicable provide reason why)

Screenshots

Acceptance criteria

QA checklist has been completed
Screenshots have been attached that cover desktop and mobile screens

Definition of done

Documentation has been updated, if applicable
A link has been provided to the originating GitHub issue (or connected to it via ZenHub)

powellkerry · 2024-10-08T15:34:12Z

I have a couple concerns:

Headings disappear on smaller screens, this is what the behavior in prod is currently. It seems like you should be able to sort table data on all screens, especially smaller screens where there is less real-estate and you want to be able to find things faster, but that is not currently possible. This may be a conversation to have with USWDS and design
The screen reader announces the sort buttons on focus as "table groups" and the instructions on how to interact are not typical for a button and don't work. I think the buttons should have more descriptive instructions and get announced as buttons. @rsmithadhoc might be able to help better on that.
The label should also probably be major for this PR as it adds functionality

rsmithadhoc · 2024-10-09T13:29:35Z

The screen reader announces the sort buttons on focus as "table groups" and the instructions on how to interact are not typical for a button and don't work. I think the buttons should have more descriptive instructions and get announced as buttons. @rsmithadhoc might be able to help better on that.

I believe those will need to be wrapped in a <button>. This should also have a title attribute with text saying "Click to sort by (column name) in (ascending/descending) order", that will read it out on a screen reader and also show a tooltip on hover.

jamigibbs · 2024-10-09T13:34:02Z

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx

@@ -47,6 +47,22 @@ export class VaTableInner {
   */
  @Prop() stacked?: boolean = false;

+  @Prop() sortdir?: string = null;
+
+  @Prop() sortindex?: number = null;


If sortdir and sortindex are public properties, can we add documentation comments for them to explain how they work? Would it also be valuable to add a story that demonstrates them?

@jamigibbs - good call - they don't need to be props, I have handled this using data- attrs and State in a refactor

jamigibbs · 2024-10-09T13:35:20Z

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx

+    composed: true,
+    bubbles: true,
+  })
+  sortTable: EventEmitter;


Is this event just for internal component use? Or do we think there's a reason that a team would want to be listening for this event so they could do something with it?

@jamigibbs it should only be useful internally

jamigibbs · 2024-10-09T13:40:27Z

packages/web-components/src/components/va-table/sort/alpha.tsx

@@ -0,0 +1,12 @@
+import { CompareFuncReturn } from "./utils";
+
+export function alphaSort(sortdir: string): CompareFuncReturn {


What does this function do? Could we add some documenting comments?

@jamigibbs fixed!

jamigibbs · 2024-10-09T13:42:20Z

packages/web-components/src/components/va-table/sort/constants.tsx

@@ -0,0 +1,37 @@
+export const months =  {
+  "january": 1,


I'm a little torn about keeping utility functions and constants in a specific component because we already have a shared place for things related to dates and utilities.

I wonder if anyone else has any thoughts about the location?

@jamigibbs I wondered this too - I moved the constants to higher level utils folder because there is a chance of some other component using them

jamigibbs · 2024-10-09T13:42:52Z

packages/web-components/src/components/va-table/sort/date.tsx

+  return month ? month : new Date(string).getTime();
+}
+
+export function dateSort(sortdir: string): CompareFuncReturn {


Can we add a small comment about what this function is doing?

@jamigibbs - fixed!

jamigibbs · 2024-10-09T13:44:25Z

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.scss

@@ -33,6 +45,14 @@ caption {
  margin-bottom: .75rem;
 }

+.th-sort-header {
+  background-color: var(--vads-color-primary-alt-light) !important;


Are there any specific USWDS classes from their table module that could be used instead of custom CSS? Same question for the .sorted-data class below too.

@jamigibbs - yes there are! fixed!

jamigibbs · 2024-10-09T13:54:17Z

Headings disappear on smaller screens, this is what the behavior in prod is currently. It seems like you should be able to sort table data on all screens, especially smaller screens where there is less real-estate and you want to be able to find things faster, but that is not currently possible. This may be a conversation to have with USWDS and design

@powellkerry Is what you're describing the same thing as this issue?

Responsive v1/v3 va-table loses headers on smaller screens vets-design-system-documentation#3327

…fairs/component-library into sortable-table

it-harrison · 2024-10-09T21:58:17Z

https://sortable-table--65a6e2ed2314f7b8f98609d8.chromatic.com

@powellkerry @jamigibbs good call on the disappearing headers; I pushed a commit so that they don't disappear for a sortable table

…fairs/component-library into sortable-table

it-harrison · 2024-10-11T19:35:08Z

The screen reader announces the sort buttons on focus as "table groups" and the instructions on how to interact are not typical for a button and don't work. I think the buttons should have more descriptive instructions and get announced as buttons. @rsmithadhoc might be able to help better on that.

I believe those will need to be wrapped in a <button>. This should also have a title attribute with text saying "Click to sort by (column name) in (ascending/descending) order", that will read it out on a screen reader and also show a tooltip on hover.

@rsmithadhoc instead of a button I used a span with a role="button" so that I didn't have to style the button - VoiceOver was the same but want to make sure this is okay.

rsmithadhoc · 2024-10-15T15:56:48Z

The screen reader announces the sort buttons on focus as "table groups" and the instructions on how to interact are not typical for a button and don't work. I think the buttons should have more descriptive instructions and get announced as buttons. @rsmithadhoc might be able to help better on that.

I believe those will need to be wrapped in a <button>. This should also have a title attribute with text saying "Click to sort by (column name) in (ascending/descending) order", that will read it out on a screen reader and also show a tooltip on hover.

@rsmithadhoc instead of a button I used a span with a role="button" so that I didn't have to style the button - VoiceOver was the same but want to make sure this is okay.

@it-harrison It should be okay functionality-wise, Andrew ran into issues recently with linting and explicitly defining a role, so that linting rule had to be disabled. I believe the outcome was that the rule had to be disabled in a handful of vets-website tests, but ideally to remove that in the future and use the actual element. I'm not sure if we would want to expand on those one-off rule disables if we don't have to. But just know you might see issues in vets-website. Here's the Slack thread on that: https://dsva.slack.com/archives/G01BJ3ESXL4/p1727383688504589?thread_ts=1727380266.191099&cid=G01BJ3ESXL4

I'll give my approval, I also have department-of-veterans-affairs/vets-design-system-documentation#2772 to give it a full a11y review as well.

jamigibbs

Good updates so far, Ian! I have just a few more observations/questions:

jamigibbs · 2024-10-15T16:29:00Z

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx

+      }
+      return (
+        <span
+          role="button"


I think USWDS is using a button with styling classes. Can we do that too here instead of using a span?

@jamigibbs - I switched to button but had to style it since the styles from uswds diverge from ours here

@department-of-veterans-affairs/platform-design-system-designers Are we aware that there is a design divergence between USWDS table and our Figma for the header buttons?

@jamigibbs - I have keyed Dan into this and he will be working on a solution going forward

jamigibbs · 2024-10-15T16:31:05Z

packages/web-components/src/components/va-table/va-table.tsx

+  /**
+   * Is the table sortable
+   */
+  @Prop() sortable: boolean = false; 


Have we tested how this behaves with pagination?

@jamigibbs there was a bug (not actually related to pagination but revealed in the pagination example) that is now fixed

it-harrison · 2024-10-17T23:16:04Z

@rsmithadhoc heads up - I went with buttons in the table header ths.

jamigibbs

Excellent work, @it-harrison!

If there is any time to add tests for the sort utility methods, there might be an opportunity to do that similar to how va-telephone has unit tests.

And with there being visual changes, let's try and have a design approval before merging too. 🙏🏼

it-harrison · 2024-10-21T21:15:31Z

https://sortable-table--65a6e2ed2314f7b8f98609d8.chromatic.com

@jamigibbs sorting utils tests added. reaching out to design for review

it-harrison · 2024-10-21T21:16:50Z

@danbrady @LWWright7 @babsdenney as per a suggestion from @jamigibbs this PR will wait to merge until one of you has had time to review it for visual changes - thanks!

danbrady · 2024-10-22T19:01:39Z

Sorry for the delay in review, @it-harrison. I'm a bit swamped. Quick question on something I notice... How come the sorted icons are different between USWDS and VADS?

USWDS:

VADS:

it-harrison · 2024-10-22T19:35:31Z

Sorry for the delay in review, @it-harrison. I'm a bit swamped. Quick question on something I notice... How come the sorted icons are different between USWDS and VADS?

USWDS:

VADS:

@danbrady no worries and good call - I used va-icon whereas uswds has their own icon set. I assumed that we should use va-icon (I think we have done so in other components that diverge in this way from uwswds) but I could be wrong.

danbrady · 2024-10-22T20:39:18Z

I see why you did that, Ian. That makes sense to me.

Maybe the USWDS uses that slim arrow instead because it looks closer to the default sort icon. Whereas the Material/VADS arrows are downright chonky. However, the USWDS should probably include those slim arrows in their icon set.

Here's three possible ideas for a way forward:

Keep the icons as we have it now (not aligned with USWDS) using these:
Use the images/unofficial icons that USWDS currently uses and not va-icon (and submit an issue to them):
Add a new, similar style sortable icon from Material to match the style of the arrows in va-icon:

@humancompanion-usds, your thoughts?

@it-harrison The rest of the design looks good. I don't think it's necessary to stop merging this based on the icon alone. We can create a follow up ticket, if needed. Thanks!

it-harrison and others added 3 commits October 1, 2024 15:02

initial sort working

2119bbc

sort working

e1116fe

Merge branch 'main' into sortable-table

1ff5d2c

it-harrison added the minor For a minor Semantic Version change label Oct 8, 2024

it-harrison marked this pull request as ready for review October 8, 2024 00:34

it-harrison requested a review from a team as a code owner October 8, 2024 00:34

jamigibbs reviewed Oct 9, 2024

View reviewed changes

jamigibbs requested review from a team and rsmithadhoc October 9, 2024 13:46

it-harrison added 3 commits October 9, 2024 11:35

Merge branch 'main' into sortable-table

193ef42

make sortable table not stack

d7d56ab

Merge branch 'sortable-table' of github.com:department-of-veterans-af…

d966bc2

…fairs/component-library into sortable-table

powellkerry and others added 3 commits October 10, 2024 15:31

Merge branch 'main' into sortable-table

b542138

address pr comments

ef05f51

Merge branch 'sortable-table' of github.com:department-of-veterans-af…

071e886

…fairs/component-library into sortable-table

it-harrison added the major Major change in semantic versioning label Oct 11, 2024

rsmithadhoc approved these changes Oct 15, 2024

View reviewed changes

jamigibbs reviewed Oct 15, 2024

View reviewed changes

it-harrison added 4 commits October 17, 2024 13:01

tweak sort order check

e7c2c2b

fix sort bug

a3b68d4

switch span to button

7faacf4

sync with main

a653859

fix tests

9148c9a

it-harrison removed the minor For a minor Semantic Version change label Oct 18, 2024

it-harrison mentioned this pull request Oct 20, 2024

Add Sorting to Borderless Table - Development department-of-veterans-affairs/vets-design-system-documentation#2771

Closed

14 tasks

remove obsolete attr

07f83b7

jamigibbs approved these changes Oct 21, 2024

View reviewed changes

jamigibbs mentioned this pull request Oct 21, 2024

va-table: update va-table-inner to properly scope table header row to columns #1370

Merged

18 tasks

add tests for sort utils

46f8273

danbrady approved these changes Oct 22, 2024

View reviewed changes

Merge branch 'main' into sortable-table

0c1c346

it-harrison merged commit 266e660 into main Oct 23, 2024
8 checks passed

it-harrison deleted the sortable-table branch October 23, 2024 14:30

jamigibbs changed the title ~~Sortable table~~ va-table: add sorting variation Oct 23, 2024

jamigibbs added minor For a minor Semantic Version change and removed major Major change in semantic versioning labels Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

va-table: add sorting variation #1358

va-table: add sorting variation #1358

it-harrison commented Oct 7, 2024 •

edited

Loading

powellkerry commented Oct 8, 2024 •

edited

Loading

rsmithadhoc commented Oct 9, 2024

jamigibbs Oct 9, 2024

it-harrison Oct 11, 2024

jamigibbs Oct 9, 2024

it-harrison Oct 11, 2024

jamigibbs Oct 9, 2024

it-harrison Oct 11, 2024

jamigibbs Oct 9, 2024

it-harrison Oct 11, 2024

jamigibbs Oct 9, 2024

it-harrison Oct 11, 2024

jamigibbs Oct 9, 2024 •

edited

Loading

it-harrison Oct 11, 2024

jamigibbs commented Oct 9, 2024

it-harrison commented Oct 9, 2024

it-harrison commented Oct 11, 2024

rsmithadhoc commented Oct 15, 2024 •

edited

Loading

jamigibbs left a comment •

edited

Loading

jamigibbs Oct 15, 2024

it-harrison Oct 17, 2024

jamigibbs Oct 18, 2024 •

edited

Loading

it-harrison Oct 20, 2024

jamigibbs Oct 15, 2024

it-harrison Oct 17, 2024

it-harrison commented Oct 17, 2024

jamigibbs left a comment

it-harrison commented Oct 21, 2024

it-harrison commented Oct 21, 2024

danbrady commented Oct 22, 2024

it-harrison commented Oct 22, 2024

danbrady commented Oct 22, 2024

		@@ -0,0 +1,12 @@
		import { CompareFuncReturn } from "./utils";

		export function alphaSort(sortdir: string): CompareFuncReturn {

va-table: add sorting variation #1358

va-table: add sorting variation #1358

Conversation

it-harrison commented Oct 7, 2024 • edited Loading

Chromatic

Description

QA Checklist

Screenshots

Acceptance criteria

Definition of done

powellkerry commented Oct 8, 2024 • edited Loading

rsmithadhoc commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamigibbs Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamigibbs commented Oct 9, 2024

it-harrison commented Oct 9, 2024

it-harrison commented Oct 11, 2024

rsmithadhoc commented Oct 15, 2024 • edited Loading

jamigibbs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamigibbs Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

it-harrison commented Oct 17, 2024

jamigibbs left a comment

Choose a reason for hiding this comment

it-harrison commented Oct 21, 2024

it-harrison commented Oct 21, 2024

danbrady commented Oct 22, 2024

it-harrison commented Oct 22, 2024

danbrady commented Oct 22, 2024

it-harrison commented Oct 7, 2024 •

edited

Loading

powellkerry commented Oct 8, 2024 •

edited

Loading

jamigibbs Oct 9, 2024 •

edited

Loading

rsmithadhoc commented Oct 15, 2024 •

edited

Loading

jamigibbs left a comment •

edited

Loading

jamigibbs Oct 18, 2024 •

edited

Loading