Table: Create table components #695

jialin-he · 2024-03-02T02:40:12Z

This change: (check at least one)

Is this a breaking change? (check one)

Yes
No

Is the: (complete all)

Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
Test suite(s) passing?
Code coverage maximal?
Changeset added?
Component status page up to date?

What does this change address?

Resolves #355 ; Create a basic table component

How does this change work?
A table component that takes in user input data and display with pagination.

changeset-bot · 2024-03-02T02:40:15Z

🦋 Changeset detected

Latest commit: 2161b85

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@ithaka/pharos	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-03-02T02:43:10Z

size-limit report 📦

Path	Size
packages/pharos/lib/index.js	63.79 KB (+1.37% 🔺)

packages/pharos/src/components/table/pharos-table.scss

daneah

Looks as though there's a little layout blip between when the Previous button is shown in the pagination vs. not.

Screen.Recording.2024-03-05.at.10.19.49.mov

packages/pharos/src/components/table/pharos-table.ts

daneah · 2024-03-05T15:11:44Z

packages/pharos/src/components/table/pharos-table.ts

+  /* eslint-disable @typescript-eslint/no-explicit-any */
+  public columns: any[] = [];


I'm just spitballing here, but it could be interesting to think about a system where the column spec specifies the expected data type of each column, and that the types would guide telling a consumer if they've supplied data of the wrong types. At a minimum, perhaps we could make an alias type to avoid all the eslint-disables?

I've been thinking about your suggestion Dane. I wonder if it is possible in some cases the content in a column can be in multiple types. Do we require the consumer to provide all possible types? Will that be too heavy load for the consumer?

I would generally hope that if we're displaying tabular data, that within a column the data type is consistent (or can be coerced to be consistent by way of string, minimally). There's a tradeoff here between safety and tedium as you rightly call out...I don't really have strong feelings on this, and it can definitely be iterated on, so I don't want it to hold us up here too much!

packages/pharos/src/components/table/pharos-table.ts

packages/pharos/src/components/table/pharos-table.test.ts

Co-authored-by: Dane Hillard <github@danehillard.com>

kentjas1 · 2024-03-08T18:47:16Z

packages/pharos/src/components/table/pharos-table.ts

+  public pageSizeOptions: number[] = [50, 100];
+
+  @property({ type: String, reflect: true, attribute: 'caption' })
+  public caption: string = '';


I understand we need the caption for accessibility reasons, I'm curious if there should be any ability to visually hide it in certain cases or is the expectation should be having the practice of always showing a table caption?

☝️ @sirrah-tam might have some insight into this, I think it could be "visually hidden" as long as it is still accessible to screen readers

It would be great to be able to include an attribute that auto-visually-hides the caption as that does appear to be pretty common in other examples I'm finding. Similar to how the <title> element gives a SVG it's accessible name (accNam), the gives the Table it's accName, helpful when users of AT are reviewing the contents of a page. Visually hiding the captions would mean that a sighted user would have to use other information on the page to understand the context and purpose of the table, ex page title or heading preceding the table.

If I understand correctly, we want to add another attribute, something like auto-visually-hide = true, to control if we want to visually hide the caption? Does that mean we should make the caption attribute a required attribute so that we always have a accessible name for the table?

I do feel like the caption should be required to enforce always having accessible tables generated but would leave that up to @sirrah-tam and the other wickies to officially answer yes/no.

The caption element is not required per WCAG and there are some cases where the caption may not be needed, especially for simple tables where there is already a heading that precedes the table. However, I do like that we'd kind of take a stand that all tables need a caption by requiring it for our design system. I would however say we shouldn't have it hidden by default and instead make the user explicitly hide it.

sirrah-tam · 2024-03-12T19:35:39Z

packages/pharos/src/components/table/pharos-table.scss

+}
+
+.hide-table-caption {
+  visibility: hidden;


This CSS style will actually hide the content from a screen reader, similar to display: none. It's better to use the following set of styles when trying to visually hide content:

.visually-hidden:not(:focus):not(:active) { clip: rect(0 0 0 0); clip-path: inset(50%); height: 1px; overflow: hidden; position: absolute; white-space: nowrap; width: 1px; }

Reference: https://www.scottohara.me/blog/2017/04/14/inclusively-hidden.html

I think this mixin is what you want within pharos

pharos/packages/pharos/src/utils/scss/_mixins.scss

Line 130 in e396a02

@mixin hidden {

sirrah-tam · 2024-03-12T19:38:45Z

packages/pharos/src/components/table/pharos-table.ts

+  public pageSizeOptions: number[] = [50, 100];
+
+  @property({ type: String, reflect: true, attribute: 'caption' })
+  public caption: string = '';


The caption element is not required per WCAG and there are some cases where the caption may not be needed, especially for simple tables where there is already a heading that precedes the table. However, I do like that we'd kind of take a stand that all tables need a caption by requiring it for our design system. I would however say we shouldn't have it hidden by default and instead make the user explicitly hide it.

daneah · 2024-03-13T15:30:12Z

packages/pharos/src/components/table/pharos-table.scss

+.table-controls {
+  display: flex;
+  justify-content: space-between;
+  align-items: center;
+  margin-top: var(--pharos-spacing-one-half-x);
+
+  .item-per-page-wrapper {
+    display: flex;
+    align-items: center;
+    column-gap: var(--pharos-spacing-one-half-x);
+
+    .item-per-page-selector {
+      width: 75px;
+    }
+  }
+
+  .pagination {
+    height: var(--pharos-spacing-one-and-a-half-x);
+  }
+}


Typically we try to use a more BEM-like approach rather than deeply nested selectors. Because this isn't automated and is somewhat subjective currently, let's let it stand as a "TODO" for later 😄

packages/pharos/src/components/table/pharos-table.ts

Co-authored-by: Dane Hillard <github@danehillard.com>

brentswisher · 2024-03-13T17:04:57Z

packages/pharos/src/components/table/pharos-table.ts

+  public rowData: RowData[] = [];
+
+  @property({ type: Boolean, reflect: true, attribute: 'hide-pagination' })
+  public hidePagination: boolean = true;


Might just be just personal opinion, but thoughts on making this showPagination instead and defaulting it to false? Setting hidePagination="false" to show the pagination seems like a more complicated mental model than just showPagination="true" or showPagination="false" ?

Yeah I feel showPagination makes more sense, updated!

brentswisher · 2024-03-13T17:26:13Z

packages/pharos/src/components/table/pharos-table.test.ts

+  it('is accessible', async () => {
+    await expect(component).to.be.accessible();
+  });
+


Might be nice to add a unit test like this one which verifies it throws an error when there is no caption?

packages/pharos/src/components/table/pharos-table.test.ts

brentswisher · 2024-03-13T17:29:53Z

packages/pharos/src/components/table/pharos-table.ts

+  protected override firstUpdated(): void {
+    this._pageSize = this.hidePagination ? this.rowData.length : this.pageSizeOptions[0];
+    this.totalResults =
+      this.hidePagination || !this.totalResults ? this.rowData.length : this.totalResults;


Why do we care about hidePagination here?

Oh I guess we don't need it here!

brentswisher

@jialin-he Just a couple of nits/questions, but this is looking awesome 🎉

Co-authored-by: Brent Swisher <brent@brentswisher.com>

sirrah-tam

Small request to change the reference to hideCaption vs hideCaptionVisually as more concise way of setting that attribute.

packages/pharos/src/components/table/pharos-table.ts

Co-authored-by: Mat Harris <mat.harris@ithaka.org>

jialin-he added 5 commits February 23, 2024 15:11

feat(table): add pharos table component

3844c8d

feat(table): add more to tables

ed0dc5b

feat(table): implement page size dropdown

ba2cba8

feat(table): add react storybook

b98a7d8

feat(table): add unit tests to table

7376c7b

jialin-he added the status/code review label Mar 2, 2024

jialin-he requested review from kentjas1 and gbassham March 2, 2024 02:40

jialin-he requested a review from a team as a code owner March 2, 2024 02:40

jialin-he requested review from daneah, sirrah-tam and mtorres3 and removed request for a team March 2, 2024 02:40

jialin-he and others added 4 commits March 1, 2024 21:43

docs(table): add changeset

0bd9327

feat(table): update storybook

fbed790

Merge branch 'develop' into feat/pharos-table

0efbf1e

feat(table): improve storybook reactivity

d182e5a

mtorres3 reviewed Mar 4, 2024

View reviewed changes

packages/pharos/src/components/table/pharos-table.scss Outdated Show resolved Hide resolved

mtorres3 reviewed Mar 4, 2024

View reviewed changes

packages/pharos/src/components/table/pharos-table.scss Outdated Show resolved Hide resolved

mtorres3 approved these changes Mar 4, 2024

View reviewed changes

jialin-he added 2 commits March 4, 2024 12:04

feat(table): add accessibility update

b2612d0

feat(table): use pharos color

f3861a1

jialin-he requested a review from brentswisher March 4, 2024 17:20

jialin-he added 2 commits March 5, 2024 10:00

feat(table): fix bug

1a99376

feat(table): fix build issue

31f2164

daneah reviewed Mar 5, 2024

View reviewed changes

jialin-he and others added 2 commits March 5, 2024 13:10

Update packages/pharos/src/components/table/pharos-table.ts

878db09

Co-authored-by: Dane Hillard <github@danehillard.com>

Update packages/pharos/src/components/table/pharos-table.ts

1bcc071

Co-authored-by: Dane Hillard <github@danehillard.com>

jialin-he and others added 3 commits March 7, 2024 09:55

Update packages/pharos/src/components/table/pharos-table.test.ts

a54bfba

Co-authored-by: Dane Hillard <github@danehillard.com>

Update packages/pharos/src/components/table/pharos-table.test.ts

0a1f1fe

Co-authored-by: Dane Hillard <github@danehillard.com>

feat(table): update row data type

01f4343

kentjas1 reviewed Mar 8, 2024

View reviewed changes

jialin-he and others added 3 commits March 12, 2024 13:36

feat(table): update unit test

9bef87b

feat(table): add hide caption visually attribute

10aa6bb

Merge branch 'develop' into feat/pharos-table

50d1727

sirrah-tam reviewed Mar 12, 2024

View reviewed changes

jialin-he and others added 5 commits March 12, 2024 16:01

feat(table): update visually hidden css

7d8920f

feat(table): use hidden mixin

0671f2d

feat(table): fix lint and bug

10a15ef

feat(table): fix unit tests

1f1b2ac

Merge branch 'develop' into feat/pharos-table

c38abda

daneah approved these changes Mar 13, 2024

View reviewed changes

Update packages/pharos/src/components/table/pharos-table.ts

c135b75

Co-authored-by: Dane Hillard <github@danehillard.com>

brentswisher reviewed Mar 13, 2024

View reviewed changes

packages/pharos/src/components/table/pharos-table.test.ts Outdated Show resolved Hide resolved

brentswisher reviewed Mar 13, 2024

View reviewed changes

Update packages/pharos/src/components/table/pharos-table.test.ts

e416547

Co-authored-by: Brent Swisher <brent@brentswisher.com>

sirrah-tam reviewed Mar 13, 2024

View reviewed changes

packages/pharos/src/components/table/pharos-table.ts Outdated Show resolved Hide resolved

packages/pharos/src/components/table/pharos-table.ts Outdated Show resolved Hide resolved

packages/pharos/src/components/table/pharos-table.ts Outdated Show resolved Hide resolved

jialin-he and others added 3 commits March 13, 2024 14:28

Update packages/pharos/src/components/table/pharos-table.ts

4b6fcde

Co-authored-by: Mat Harris <mat.harris@ithaka.org>

Update packages/pharos/src/components/table/pharos-table.ts

bf20c8e

Co-authored-by: Mat Harris <mat.harris@ithaka.org>

Update packages/pharos/src/components/table/pharos-table.ts

b010390

Co-authored-by: Mat Harris <mat.harris@ithaka.org>

sirrah-tam approved these changes Mar 13, 2024

View reviewed changes

feat(table): update attribute name and add unit test

2161b85

jialin-he merged commit b864531 into develop Mar 14, 2024
6 of 11 checks passed

jialin-he deleted the feat/pharos-table branch March 14, 2024 12:35

github-actions bot mentioned this pull request Mar 14, 2024

Version Packages #721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table: Create table components #695

Table: Create table components #695

jialin-he commented Mar 2, 2024 •

edited by daneah

changeset-bot bot commented Mar 2, 2024 •

edited

github-actions bot commented Mar 2, 2024 •

edited

daneah left a comment

daneah Mar 5, 2024

jialin-he Mar 6, 2024

jialin-he Mar 12, 2024

daneah Mar 12, 2024

kentjas1 Mar 8, 2024

brentswisher Mar 8, 2024

sirrah-tam Mar 8, 2024

jialin-he Mar 12, 2024

kentjas1 Mar 12, 2024 •

edited

jialin-he Mar 12, 2024

sirrah-tam Mar 12, 2024

sirrah-tam Mar 12, 2024 •

edited

jialin-he Mar 12, 2024

brentswisher Mar 12, 2024

jialin-he Mar 12, 2024

sirrah-tam Mar 12, 2024

daneah Mar 13, 2024

brentswisher Mar 13, 2024

jialin-he Mar 13, 2024

brentswisher Mar 13, 2024

jialin-he Mar 13, 2024

brentswisher Mar 13, 2024

jialin-he Mar 13, 2024 •

edited

brentswisher left a comment

sirrah-tam left a comment

		/* eslint-disable @typescript-eslint/no-explicit-any */
		public columns: any[] = [];

Table: Create table components #695

Table: Create table components #695

Conversation

jialin-he commented Mar 2, 2024 • edited by daneah

changeset-bot bot commented Mar 2, 2024 • edited

🦋 Changeset detected

github-actions bot commented Mar 2, 2024 • edited

size-limit report 📦

daneah left a 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

kentjas1 Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirrah-tam Mar 12, 2024 • edited

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

Choose a reason for hiding this comment

jialin-he Mar 13, 2024 • edited

Choose a reason for hiding this comment

brentswisher left a comment

Choose a reason for hiding this comment

sirrah-tam left a comment

Choose a reason for hiding this comment

jialin-he commented Mar 2, 2024 •

edited by daneah

changeset-bot bot commented Mar 2, 2024 •

edited

github-actions bot commented Mar 2, 2024 •

edited

kentjas1 Mar 12, 2024 •

edited

sirrah-tam Mar 12, 2024 •

edited

jialin-he Mar 13, 2024 •

edited