Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add DataTable and Table to drafts #2951

Merged
merged 10 commits into from
Mar 1, 2023

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Feb 28, 2023

Closes https://github.com/github/primer/issues/1814

This PR adds DataTable and Table to the drafts bundle with the following API:

  • DataTable
  • Table.*
    • Table.Container
    • Table.Title
    • Table.Subtitle
    • Table
    • Table.Head
    • Table.Body
    • Table.Row
    • Table.Header
    • Table.Cell

The idea behind this implementation is that DataTable is an alternative to Table and can slot into that position:

<Table.Container>
  <Table.Title id="table-title">Repositories</Table.Title>
  <DataTable
    aria-labelledby="table-title"
    // ...
  />
</Table.Container>

Table may also be used there and each part of Table is available under that identifier.

One risk with this approach is that Table.* contains both descendants of a Table and ancestors like Container, Title, or Subtitle. The risk here would be that certain terms may be overloaded, like in Table.Header. This would refer to a <th> but in the context of a layout within Table.Container would refer to the header region of a layout.

Changelog

New

  • Add tests for Table.* components

Changed

  • Update interface to type
  • Split up into DataTable.tsx and Table.tsx
  • Add Table and DataTable to drafts entrypoint
  • Add explicit role tags for th, tr, td for usage of display: grid for column width options

Removed

@joshblack joshblack requested review from mperrotti, colebemis and a team February 28, 2023 19:05
@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2023

🦋 Changeset detected

Latest commit: 6a6d850

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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
Copy link
Contributor

github-actions bot commented Feb 28, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 94.49 KB (0%)
dist/browser.umd.js 95.07 KB (0%)

@joshblack joshblack temporarily deployed to github-pages February 28, 2023 19:11 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2951 February 28, 2023 19:12 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2951 February 28, 2023 19:12 Inactive
@joshblack joshblack temporarily deployed to github-pages February 28, 2023 19:18 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2951 February 28, 2023 19:19 Inactive
@joshblack joshblack temporarily deployed to github-pages February 28, 2023 21:46 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2951 February 28, 2023 21:46 Inactive
'@primer/react': minor
---

Add DataTable, Table to drafts entrypoint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering when will be the best time to start exporting new components in experimental bundle instead of drafts? According to the ADR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, reached out about this over in Slack to see where that work is. For this PR I'll add it to drafts and will add it to experimental once that gets created 👀

Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting to see this take shape!

TableContainer,
TableTitle,
TableSubtitle,
} from '../DataTable'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/

export {DataTable, Table} from '../DataTable'
Copy link
Contributor

@colebemis colebemis Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we export any prop types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me as part of the definition of done. To implement this, should I include props as exports for any exported component?

@joshblack joshblack temporarily deployed to github-pages March 1, 2023 16:57 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2951 March 1, 2023 16:57 Inactive
@joshblack joshblack enabled auto-merge March 1, 2023 21:13
@github-actions github-actions bot temporarily deployed to storybook-preview-2951 March 1, 2023 21:19 Inactive
@joshblack joshblack temporarily deployed to github-pages March 1, 2023 21:19 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2951 March 1, 2023 21:19 Inactive
@joshblack joshblack disabled auto-merge March 1, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants