feat: Table roles helper and role="grid" for editable table #1365

pan-kot · 2023-07-25T10:24:56Z

Description

Encapsulated table's semantic structure in a helper module and changed the table's role to "grid" for tables with editable cells.

How has this been tested?

New unit tests to verify "grid" role for tables with editable cells.

Manual testing with VO+Chrome, VO+Safari, NVDA+Chrome, NVDA+Firefox.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2023-07-25T18:09:14Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (51c36a3) 93.47% compared to head (6a5893f) 93.48%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1365      +/-   ##
==========================================
+ Coverage   93.47%   93.48%   +0.01%     
==========================================
  Files         624      625       +1     
  Lines       16778    16819      +41     
  Branches     5550     5562      +12     
==========================================
+ Hits        15683    15724      +41     
  Misses       1022     1022              
  Partials       73       73

Files Changed	Coverage Δ
src/table/header-cell/index.tsx	`92.85% <ø> (-0.17%)`	⬇️
src/table/header-cell/utils.ts	`100.00% <ø> (ø)`
src/table/sticky-header.tsx	`100.00% <ø> (ø)`
src/table/thead.tsx	`95.34% <ø> (ø)`
src/table/body-cell/td-element.tsx	`91.30% <100.00%> (-0.70%)`	⬇️
src/table/internal.tsx	`98.65% <100.00%> (ø)`
src/table/table-role/table-role-helper.ts	`100.00% <100.00%> (ø)`

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…able-roles

just-boris · 2023-07-27T10:23:07Z

src/table/table-role/table-role-helper.ts

+ * ARIA role of the table component ("table", "grid", "treegrid") but also roles and other semantic attributes
+ * of the child elements. The TableRole helper encapsulates table's semantic structure.
+ */
+export class TableRole {


We use React and this encourages us to use pure functions instead of mutable objects.

Especially, objects that you propagate across components. Does not happen in your code, because all assign functions do not modify this, but it paves the way for bugs.

Safer option would be to propagate the role directly, without helper

On a higher level the it looks like this task is suitable to be solved with contexts

<TableContext role={hasEditableCells ? 'grid' : 'table'} />

const {getTableRowProps} = useTableContext();

We have several more props passing down the same way, so maybe if introducing table context, we can refactor more props into it

I think contexts is an overkill here - instead I am passing a stable object's reference - it is simpler and no pub/sub is required. What do you think if I make the tableRole property readonly?

You could just propagate a string role instead of the object

I think of that class as a way of binding the helper functions to the tableRole value - surely can have a module with the functions that would accept the tableRole as an additional argument, but I like the semantics of the helper more. I think it is a good idea to ensure the internal state never changes after initialisation.

Classes are not suitable for this job. If you want to know more details, welcome to my DM

I updated the helper to be a function instead, please check.

…able-roles

just-boris · 2023-07-27T12:52:06Z

I do not understand one thing

How is this better:

const tableRole = createTableRoleHelper('table');
tableRole.assignTableRowProps({ firstIndex, rowIndex });

than this

getRowProps({role: 'table', firstIndex, rowIndex })

I am trying really hard to understand the original motivation for grouping this, but I do not see it

pan-kot · 2023-07-27T13:10:41Z

I do not understand one thing

How is this better:
const tableRole = createTableRoleHelper('table');
tableRole.assignTableRowProps({ firstIndex, rowIndex });
than this
getRowProps({role: 'table', firstIndex, rowIndex })
I am trying really hard to understand the original motivation for grouping this, but I do not see it

Same as you suggested above with the contexts. When there is a single helper it can be instantiated with more properties.
I will update it to the plain set of functions for now.

just-boris · 2023-07-27T13:31:57Z

src/table/table-role/table-role-helper.ts

+export function assignTableCellProps(
+  options: { tableRole: TableRole },
+  nativeProps: React.TdHTMLAttributes<HTMLTableCellElement> = {}
+): React.TdHTMLAttributes<HTMLTableCellElement> {


On Object.assign(), the first argument receives new properties not the last. Let's avoid confusing people and stick to the convention at least.

or the best solution, avoid using mutable objects at all, like this

{...getTableProps(tableRole)} // the object gets inside that function, no chances of overwriting something that we should not

Or for table cell

nativeAttributes.role = getTableCellRole(tableRole)

Sure, can change the attributes order. The assignment is made to avoid objects cloning which is then has to be done for all cells and is unnecessary. Sure, can create 10 utility methods to set all attributes individually but the point was to make a single place where the semantic attributes are assigned.

You still can group all of this attribute management in a single file without mutating objects.

The ask is only about avoiding mutations, the grouping of the code is up to you

pan-kot added 4 commits July 25, 2023 11:56

Created table role helpers

d338873

Use grid role for table with inline edit

e19cb40

fix unit tests

2092f33

Fix table aria-label propagation

cab9495

pan-kot deployed to preview July 25, 2023 18:08 — with GitHub Actions View deployment

pan-kot added 2 commits July 26, 2023 09:53

additional tests

3809f64

refactor and document table helper

5045ab2

pan-kot changed the title ~~Table roles~~ chore: Table roles helper Jul 26, 2023

pan-kot changed the title ~~chore: Table roles helper~~ chore: Table roles helper and role="grid" for editable table Jul 26, 2023

pan-kot deployed to preview July 26, 2023 08:24 — with GitHub Actions View deployment

Merge branch 'main' of github.com:cloudscape-design/components into t…

c35aca2

…able-roles

pan-kot deployed to preview July 26, 2023 13:18 — with GitHub Actions View deployment

pan-kot marked this pull request as ready for review July 26, 2023 14:17

pan-kot requested a review from a team as a code owner July 26, 2023 14:17

pan-kot requested review from just-boris and removed request for a team July 26, 2023 14:17

pan-kot changed the title ~~chore: Table roles helper and role="grid" for editable table~~ feat: Table roles helper and role="grid" for editable table Jul 26, 2023

just-boris reviewed Jul 27, 2023

View reviewed changes

pan-kot added 2 commits July 27, 2023 14:08

Merge branch 'main' of github.com:cloudscape-design/components into t…

d29c28c

…able-roles

new TableRole -> createTableRoleHelper

53878a5

pan-kot deployed to preview July 27, 2023 12:34 — with GitHub Actions View deployment

replace table role helper with helper functions

811f46a

just-boris reviewed Jul 27, 2023

View reviewed changes

pan-kot deployed to preview July 27, 2023 13:32 — with GitHub Actions View deployment

replace assigns with get to avoid mutations

6a5893f

just-boris approved these changes Jul 27, 2023

View reviewed changes

pan-kot deployed to preview July 27, 2023 14:51 — with GitHub Actions View deployment

pan-kot merged commit 4b2c67b into main Jul 27, 2023
24 checks passed

pan-kot deleted the table-roles branch July 27, 2023 15:55

pan-kot mentioned this pull request Nov 4, 2024

feat: Table grid navigation #1893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Table roles helper and role="grid" for editable table #1365

feat: Table roles helper and role="grid" for editable table #1365

pan-kot commented Jul 25, 2023 •

edited

Loading

codecov bot commented Jul 25, 2023 •

edited

Loading

just-boris Jul 27, 2023

just-boris Jul 27, 2023 •

edited

Loading

pan-kot Jul 27, 2023

just-boris Jul 27, 2023

pan-kot Jul 27, 2023

just-boris Jul 27, 2023

pan-kot Jul 27, 2023

just-boris commented Jul 27, 2023

pan-kot commented Jul 27, 2023

just-boris Jul 27, 2023 •

edited

Loading

just-boris Jul 27, 2023

pan-kot Jul 27, 2023

just-boris Jul 27, 2023

pan-kot Jul 27, 2023

feat: Table roles helper and role="grid" for editable table #1365

feat: Table roles helper and role="grid" for editable table #1365

Conversation

pan-kot commented Jul 25, 2023 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Jul 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

just-boris Jul 27, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

just-boris commented Jul 27, 2023

pan-kot commented Jul 27, 2023

just-boris Jul 27, 2023 • 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

Choose a reason for hiding this comment

pan-kot commented Jul 25, 2023 •

edited

Loading

codecov bot commented Jul 25, 2023 •

edited

Loading

just-boris Jul 27, 2023 •

edited

Loading

just-boris Jul 27, 2023 •

edited

Loading