Migrate Table to TypeScript #852

robinmetral · 2021-03-19T20:02:42Z

Addresses #516.

Purpose

Migrate the Table component to TypeScript.

Approach and changes

Migrate Table component and sub-components to TypeScript
Various small fixes and improvements discovered during the migration ✨

Definition of done

vercel · 2021-03-19T20:02:45Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/7rPZT8ctLYQekHt9v7zqNFUEQN41
✅ Preview: https://oss-circuit-ui-git-feat-migrate-table-to-typescript-sumup.vercel.app

sumup-clark · 2021-03-19T20:02:46Z

Hey @robinmetral,
we are super excited that you are contributing! 🎉There's one more thing you need to do. Please accept our Contributor License Agreement. It helps you and us to collaborate on clear terms and focus on what we love most: code.

Thanks!

changeset-bot · 2021-03-23T16:55:16Z

🦋 Changeset detected

Latest commit: b3c6637

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

This PR includes changesets to release 1 package

Name	Type
@sumup/circuit-ui	Patch

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

This also passes rowheaders={false} to the rendered component because it's not what's being testes here. With row headers, the first column of the table receives a "rowheader" WAI role instead of the "cell" role that we're targeting in the tests.

src/components/Table/components/TableHeader/TableHeader.js

This commit also removes a custom keyboard event listener. Enter and space key down events are already handled as a click event. We were effectively triggering onClick twice - this was discovered by switching from fireEvent to userEvent. I'm leaving the test covering the keyboard event logic in the spec.

It will be assigned a value via defaultProps but should be optional from the point of view of its parents. This was breaking specs that were not explicitly passing a testid

codecov · 2021-03-31T16:02:02Z

Codecov Report

Merging #852 (b3c6637) into canary (17714b7) will decrease coverage by 0.01%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##           canary     #852      +/-   ##
==========================================
- Coverage   91.91%   91.89%   -0.02%     
==========================================
  Files         161      161              
  Lines        2820     2888      +68     
  Branches      717      751      +34     
==========================================
+ Hits         2592     2654      +62     
- Misses        203      208       +5     
- Partials       25       26       +1

Impacted Files	Coverage Δ
packages/circuit-ui/components/Table/constants.ts	`100.00% <ø> (ø)`
packages/circuit-ui/components/Table/utils.ts	`92.00% <92.00%> (ø)`
packages/circuit-ui/components/Table/Table.tsx	`94.62% <92.68%> (ø)`
...omponents/Table/components/TableHead/TableHead.tsx	`91.17% <92.85%> (ø)`
...omponents/Table/components/SortArrow/SortArrow.tsx	`100.00% <100.00%> (ø)`
...omponents/Table/components/TableBody/TableBody.tsx	`100.00% <100.00%> (ø)`
...components/Table/components/TableCell/TableCell.ts	`100.00% <100.00%> (ø)`
...nents/Table/components/TableHeader/TableHeader.tsx	`100.00% <100.00%> (ø)`
.../components/Table/components/TableRow/TableRow.tsx	`100.00% <100.00%> (ø)`
... and 1 more

…grate-table-to-typescript

robinmetral · 2021-03-31T16:37:19Z

☝️ @connor-baer I just merged canary into this branch, tried to rebase but too many things moved around - hope that's fine with you 🙂

Just need to migrate a couple more Table sub-components, clean up types across components (import them from one place instead of redeclaring), and this will be ready for review (probably by EOW!) 🎉

packages/circuit-ui/components/Table/Table.spec.tsx

packages/circuit-ui/components/Table/Table.tsx

packages/circuit-ui/components/Table/components/TableBody/TableBody.spec.tsx

packages/circuit-ui/components/Table/components/TableCell/TableCell.ts

packages/circuit-ui/components/Table/components/TableHead/TableHead.tsx

packages/circuit-ui/components/Table/components/TableRow/TableRow.spec.tsx

packages/circuit-ui/components/Table/components/TableRow/TableRow.tsx

packages/circuit-ui/components/Table/utils.ts

robinmetral · 2021-04-07T17:20:17Z

Codecov is suddenly unhappy, I think it's because of the null and undefined checks that we added to utils.ts to make the TS compiler happy. I think we can ignore those, it's really an internal check and shouldn't happen 🙂

connor-baer

Thank you so, so much for this fantastic contribution! I know how much work went into it 🙌🏻👏🏻

packages/circuit-ui/components/Table/Table.tsx

packages/circuit-ui/components/Table/components/SortArrow/SortArrow.tsx

packages/circuit-ui/components/Table/components/TableBody/TableBody.tsx

packages/circuit-ui/components/Table/components/TableCell/TableCell.ts

packages/circuit-ui/components/Table/components/TableHead/TableHead.tsx

robinmetral

Thank you so much for the review! 🙌 I'm pushing most fixes in a minute, will follow-up in the individual comment threads for the rest

packages/circuit-ui/components/Table/Table.tsx

packages/circuit-ui/components/Table/components/SortArrow/SortArrow.tsx

packages/circuit-ui/components/Table/components/TableBody/TableBody.tsx

packages/circuit-ui/components/Table/components/TableRow/TableRow.tsx

packages/circuit-ui/components/Table/__snapshots__/Table.spec.tsx.snap

robinmetral

@connor-baer I think I addressed all the comments! I'll also check the keyboard events on different browsers now.

You can review the changes in this view (there are two commits, I had to push an extra small fix)

packages/circuit-ui/components/Table/components/TableHeader/TableHeader.tsx

…apper

connor-baer

Thank you for addressing all my comments! Looks great 🤩

.changeset/tender-poets-enjoy.md

Co-authored-by: Connor Bär <connor-baer@users.noreply.github.com>

robinmetral mentioned this pull request Mar 19, 2021

[RFC] Typescript #516

Closed

49 tasks

Robin Metral added 3 commits March 23, 2021 17:32

feat(components): migrate core Table component to TypeScript

2637e06

fix(tests): fix unused variable error and update snapshots

2936092

feat(components): migrate SortArrow to TypeScript

6f515d4

robinmetral force-pushed the feat/migrate-table-to-typescript branch from 314c380 to 6f515d4 Compare March 23, 2021 16:55

vercel bot deployed to Preview March 23, 2021 16:55 View deployment

connor-baer added this to the v2.x milestone Mar 23, 2021

feat(components): migrate TableCell to TypeScript

bef9446

vercel bot deployed to Preview March 25, 2021 17:41 View deployment

Robin Metral added 2 commits March 26, 2021 17:28

fix: forward scope prop to table header cells

e5db60a

vercel bot deployed to Preview March 26, 2021 16:42 View deployment

robinmetral commented Mar 26, 2021

View reviewed changes

src/components/Table/components/TableHeader/TableHeader.js Outdated Show resolved Hide resolved

connor-baer added the ♿ accessibility Improves usability label Mar 26, 2021

Robin Metral added 3 commits March 30, 2021 18:27

fix(tests): update snapshots

76740b9

fix(components): mark data-testid prop in TableCell as optional

0d4cc57

It will be assigned a value via defaultProps but should be optional from the point of view of its parents. This was breaking specs that were not explicitly passing a testid

vercel bot deployed to Preview March 30, 2021 16:33 View deployment

feat(components): migrate TableHeader to TypeScript

269f817

vercel bot deployed to Preview March 31, 2021 15:57 View deployment

Merge branch 'canary' of github.com:sumup-oss/circuit-ui into feat/mi…

4d3ead7

…grate-table-to-typescript

vercel bot deployed to Preview March 31, 2021 16:28 View deployment

feat(components): migrate TableHead to TypeScript

46da2c8

vercel bot deployed to Preview April 6, 2021 08:43 View deployment

feat(components): migrate TableBody to TypeScript

bd6b3ed

cleanup: remove leftover comments in utils.ts

afc124d

robinmetral commented Apr 7, 2021

View reviewed changes

robinmetral requested a review from connor-baer April 7, 2021 17:14

robinmetral marked this pull request as ready for review April 7, 2021 17:14

vercel bot deployed to Preview April 7, 2021 17:15 View deployment

connor-baer reviewed Apr 9, 2021

View reviewed changes

robinmetral commented Apr 9, 2021

View reviewed changes

fix: address code review feedback

e0ddf95

vercel bot deployed to Preview April 9, 2021 14:41 View deployment

robinmetral commented Apr 9, 2021

View reviewed changes

packages/circuit-ui/components/Table/__snapshots__/Table.spec.tsx.snap Show resolved Hide resolved

fix: revert renaming of height attr and update types

c7c61e9

vercel bot deployed to Preview April 9, 2021 14:48 View deployment

robinmetral commented Apr 9, 2021

View reviewed changes

packages/circuit-ui/components/Table/components/TableHeader/TableHeader.tsx Outdated Show resolved Hide resolved

robinmetral requested a review from connor-baer April 9, 2021 14:53

Robin Metral added 2 commits April 12, 2021 13:12

fix: reuse types from component props in static TableHeader methods

30a8578

fix: make TableProps extend HTML table element and spread props to wr…

8c5eae5

…apper

vercel bot deployed to Preview April 12, 2021 11:34 View deployment

connor-baer approved these changes Apr 12, 2021

View reviewed changes

Add changesets

aae153e

vercel bot deployed to Preview April 12, 2021 15:54 View deployment

connor-baer reviewed Apr 12, 2021

View reviewed changes

.changeset/tender-poets-enjoy.md Outdated Show resolved Hide resolved

Specify that changeset applies to the Table component

b3c6637

Co-authored-by: Connor Bär <connor-baer@users.noreply.github.com>

vercel bot deployed to Preview April 12, 2021 15:59 View deployment

robinmetral changed the title ~~feat(components): migrate Table to TypeScript~~ Migrate Table to TypeScript Apr 12, 2021

robinmetral merged commit 95be224 into canary Apr 12, 2021

robinmetral deleted the feat/migrate-table-to-typescript branch April 12, 2021 16:11

This was referenced Apr 15, 2021

Version Packages #891

Merged

Version Packages #897

Merged

robinmetral mentioned this pull request Jun 2, 2021

Improve Table component API #960

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Table to TypeScript #852

Migrate Table to TypeScript #852

robinmetral commented Mar 19, 2021 •

edited

vercel bot commented Mar 19, 2021 •

edited

sumup-clark bot commented Mar 19, 2021

changeset-bot bot commented Mar 23, 2021 •

edited

codecov bot commented Mar 31, 2021 •

edited

robinmetral commented Mar 31, 2021

robinmetral commented Apr 7, 2021

connor-baer left a comment

robinmetral left a comment

robinmetral left a comment

connor-baer left a comment

Migrate Table to TypeScript #852

Migrate Table to TypeScript #852

Conversation

robinmetral commented Mar 19, 2021 • edited

Purpose

Approach and changes

Definition of done

vercel bot commented Mar 19, 2021 • edited

sumup-clark bot commented Mar 19, 2021

changeset-bot bot commented Mar 23, 2021 • edited

🦋 Changeset detected

codecov bot commented Mar 31, 2021 • edited

Codecov Report

robinmetral commented Mar 31, 2021

robinmetral commented Apr 7, 2021

connor-baer left a comment

Choose a reason for hiding this comment

robinmetral left a comment

Choose a reason for hiding this comment

robinmetral left a comment

Choose a reason for hiding this comment

connor-baer left a comment

Choose a reason for hiding this comment

robinmetral commented Mar 19, 2021 •

edited

vercel bot commented Mar 19, 2021 •

edited

changeset-bot bot commented Mar 23, 2021 •

edited

codecov bot commented Mar 31, 2021 •

edited