fix: DataGrid reformatted to `table` and aria-sort added by dreamwasp · Pull Request #2998 · Codecademy/gamut

dreamwasp · 2024-12-16T21:42:00Z

Overview

Refactors the DataTable + DataList components to properly be semantic HTML tables and adds aria-sort to the appropriate controls

PR Checklist

Related to designs:
Related to JIRA ticket: GM-945
I have run this code to verify it works
This PR includes unit tests for the code change
This PR includes testing instructions tests for the code change
The alpha package of this PR is passing end-to-end tests in all relevant Codecademy repositories

Testing Instructions

Navigate to the DataList
Test that it still works as intended (filter, select, sort actions specifically)
Turn on VO and see that you can navigate it as a table (instructions here
You can also inspect to see no errors and in console and that it is the correct table structure
Go to DataTable
Test that it still works as intended (filter, sort actions specifically)
Turn on VO and see that you can navigate it as a table ((instructions here)
Go to List and see that they look the same as before

PR Links and Envs

Repository	PR Link	PR Env
Monolith	Monolith PR	Monolith Env
Portal	Portal Link	Portal Env

…ential

nx-cloud · 2024-12-16T21:45:50Z

View your CI Pipeline Execution ↗ for commit 0e23544.

☁️ Nx Cloud last updated this comment at 2025-01-24 22:03:03 UTC

…o cass-gm-945-i

This reverts commit e0003aa.

…to cass-gm-945-i

LinKCoding

Preview works and looks great!
VO now recognizes the table as a table and appropriately semantic rows, headers, and cells. Filtering sounds good too. And the aria-sort shows up as well for sorting.

As I was going through the code to get an understanding of the changes, I was wondering about naming conventions — to try and separate out how components could be tied to their semantic HTML counterpart.

Left some thoughts for clean up but functionally, the changes look great :)

LinKCoding · 2025-01-24T15:07:52Z

packages/gamut/src/DataList/Rows/Row.tsx


        return (
-          <ListCol {...colProps}>
+          // TO-DO: Fix type issues


Are the type issues fixed?
I don't see any locally.

oop, they are! will remove

LinKCoding · 2025-01-24T15:28:39Z

packages/gamut/src/DataList/Rows/Row.tsx

  return (
-    <ListRow expanded={expanded} renderExpanded={renderExpandedContent}>
+    <ListRow
+      as="tr"


Food for thought: since both HeaderRow.tsx and Row.tsx are specific to tables, would renaming these components and folder to be table-specific make sense?

e.g. Tables/TableHeader.tsx and Tables/TableRow.tsx respectively?

And then maybe move over TableHeader.tsx?

love it, will change

LinKCoding · 2025-01-24T15:39:12Z

packages/gamut/src/List/TableHeader.tsx

+          variant={variant}
+          scrollable={scrollable}
+          ref={ref}
+          as="tr"


Can HeaderEl be a styled tr instead? atm it's a styled div. See:
https://github.com/Codecademy/gamut/blob/cass-gm-945-i/packages/gamut/src/List/elements.tsx

maybe it should be like HeaderRowEl?

going into a rabbit hole of sorts — so feel free to disregard lol
but headerVariants seem like it's not used for anything else and only applies a borderBottom to table.
So maybe lump that into the CSS styling and do away with this variant altogether?

great call out, will refactor!

i found out this is different between DataList v DataTable so keeping the variants as is but refactored to tr

LinKCoding · 2025-01-24T15:49:52Z

packages/gamut/src/DataList/Rows/HeaderRow.tsx

  return (
-    <ListHeader>
+    <TableHeader>
      <>


Is this fragment needed anymore?

LinKCoding · 2025-01-24T15:52:11Z

packages/gamut/src/List/elements.tsx

-    header: {
-      ...headerStyles,
-    },
+    // Keeping this within variants for typing purposes, we we use this behaviorally despite it not needing specific styling


nit: extra "we" in this sentence

LinKCoding · 2025-01-24T16:11:40Z

packages/gamut/src/List/List.tsx

    const content =
      isEmpty || loading ? (
        <Box
+          as="table"


is this as needed? looks like the parent element sets the as and content seems like it could be other elements

actually, can this Box and the parent Box actually be combined?

i'll test it out! i think we were using it for some positioning reason, but i think i've changed this enough that we don't need it anymore

LinKCoding · 2025-01-24T16:23:01Z

packages/gamut/src/List/ListRow.tsx

 export interface RowProps
  extends Partial<PublicListProps<ComponentProps<typeof RowEl>>> {
  header?: boolean;
+  /** This is an internal prop that is largely only used for the DataTable component */


Since RowProps are used for DataRow, used in DataGrid, would be more accurate to say used for the "DataTable and DataList components"?

LinKCoding · 2025-01-24T16:33:02Z

packages/gamut/src/List/ListRow.tsx

        ? { ...rowConfig, ...rowProps }
        : { spacing: keepSpacingWhileExpanded ? rowConfig.spacing : undefined };
    let content = children;
    const renderNumbering = isOl && renderExpanded === undefined && !onClick;


nit: is there a falsy value other than undefined for renderExpanded?

If not, maybe !renderExpanded && !onClick can be a variable, like isNotInterative?

LinKCoding · 2025-01-24T16:36:51Z

packages/gamut/src/List/elements.tsx

 );

-export const StickyColumnWrapper = styled.div(
+export const StickyColumnWrapper = styled.th(


nit: consider renaming to StickyHeaderWrapper? Or StickyHeaderColWrapper?

LinKCoding

Working and looking great tho!

codecademydev · 2025-01-24T22:01:51Z

🚀 Styleguide deploy preview ready!

https://67940dcabd733f4782173391--gamut-preview.netlify.app

Deploy Logs

codecademydev · 2025-01-24T22:01:59Z

📬Published Alpha Packages:

@codecademy/gamut@57.7.3-alpha.0e2354.0
@codecademy/gamut-kit@0.6.464-alpha.0e2354.0
@codecademy/styleguide@68.4.1-alpha.0e2354.0

dreamwasp added 3 commits December 16, 2024 16:41

fix(DataTable): fix HTML semantics

3dfe414

html semantics in better shape, need to test if renderExpanded is ess…

96dd7db

…ential

note to self

3119770

dreamwasp changed the title ~~DataTable reqork~~ DataTable rework Dec 16, 2024

dreamwasp and others added 25 commits December 17, 2024 11:03

baseline

a96c7db

fix Meta/Stories

32467aa

Merge branch 'main' into cass-gm-945-i

6c9f717

Merge branch 'main' into cass-gm-945-i

93c450a

VERY in progress datalist

7472f0c

Merge branch 'cass-gm-945-i' of ssh://github.com/Codecademy/gamut int…

f239439

…o cass-gm-945-i

datatable mostly working, need datalist

60aa496

just need to fix datalist styling

7e51ec6

start adding flexibility, need to add counted rows

7e8b748

gettin close with data list

c059814

data list working but almost certainly need to fix other stuff

d5e1a7c

put stories back

aa23feb

fixed empty table, mostly

5ea2f58

keep testing height

1bc2948

scroll working again!

d85232c

height mostly sorted

d6ca68b

rm dots

0f35cf6

scroll working and all aria issues fixed. need to test

7ef09ad

working on responsive styles for datalist, esp expandable

af6eb46

expanded mostly fixed, plenty to refactor

8c7f676

Merge branch 'main' into cass-gm-945-i

208c0b5

row expanded sorted, time to test

a1b0b1b

remove empty space

e99412b

fix table sizing

e0003aa

Revert "fix table sizing"

bb7fbd8

This reverts commit e0003aa.

dreamwasp added 2 commits January 21, 2025 16:54

fix fontWeight for table headers

f1f657f

clean up old shadow

85c5272

dreamwasp changed the title ~~DataTable rework~~ fix: DataGrid reformatted to table and aria-sort added Jan 22, 2025

dreamwasp and others added 9 commits January 22, 2025 11:47

fix layered backgrounds + border gap

49552f4

refactor done, write tests

a9ca97a

Merge branch 'main' into cass-gm-945-i

18c32aa

remove red text

25398b1

Merge branch 'cass-gm-945-i' of ssh://github.com/Codecademy/gamut in…

8e60375

…to cass-gm-945-i

fix minheight for emptystate

f5e4dea

minheight fix

bdd0123

fix some width issues

46874e8

fix disappearing filter menus

fcdec72

dreamwasp marked this pull request as ready for review January 23, 2025 21:24

dreamwasp requested a review from a team as a code owner January 23, 2025 21:24

dreamwasp requested review from BandanaKM, LinKCoding, aresnik11 and timjenkins January 23, 2025 21:24

LinKCoding reviewed Jan 24, 2025

View reviewed changes

LinKCoding approved these changes Jan 24, 2025

View reviewed changes

dreamwasp added 4 commits January 24, 2025 14:56

respond to kenny comments

d8c0f56

static hedaer table rows

12f3177

better naming

31b31c7

safer data typing for rowa

0e23544

dreamwasp added the Ship It Automerge this PR when possible label Jan 29, 2025

codecademydev merged commit 9903099 into main Jan 29, 2025
15 of 16 checks passed

codecademydev removed the Ship It Automerge this PR when possible label Jan 29, 2025

codecademydev deleted the cass-gm-945-i branch January 29, 2025 16:44

Conversation

dreamwasp commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

PR Checklist

Testing Instructions

PR Links and Envs

Uh oh!

nx-cloud bot commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinKCoding left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinKCoding left a comment

Choose a reason for hiding this comment

Uh oh!

codecademydev commented Jan 24, 2025

Uh oh!

codecademydev commented Jan 24, 2025

📬Published Alpha Packages:

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dreamwasp commented Dec 16, 2024 •

edited

Loading

nx-cloud bot commented Dec 16, 2024 •

edited

Loading

LinKCoding left a comment •

edited

Loading