Skip to content

Add DataTable component#1687

Open
virajsanghvi wants to merge 2 commits intoopensearch-project:2.x-alphafrom
virajsanghvi:oui2-data-table
Open

Add DataTable component#1687
virajsanghvi wants to merge 2 commits intoopensearch-project:2.x-alphafrom
virajsanghvi:oui2-data-table

Conversation

@virajsanghvi
Copy link
Copy Markdown
Collaborator

Description

This is an attempt at a generic datatable based on https://ui.shadcn.com/docs/components/data-table and implemented by ai. We need to do a better evaluation of what functionality/interface we want and update this, but figured this is a decent start.

image

Issues Resolved

N/A

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Viraj Sanghvi <vsanghvi@gmail.com>
Signed-off-by: Viraj Sanghvi <virajs@amazon.com>
@virajsanghvi virajsanghvi marked this pull request as ready for review January 1, 2026 00:00
return (
<div
data-slot="data-table-column-header"
className={cn('oui:flex oui:items-center oui:space-x-2', className)}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Once we start consuming OUI 2.0, i thiunk we should enable devs in OSD to use tailwind classes. But one concern, why was it decided to use the oui: prefix? What if we just use tw:? (ref: https://tailwindcss.com/docs/styling-with-utility-classes#using-the-prefix-option)

One of the pros of using tailwind is that it appears it is the industry-accepted way of handling css in FE, so this can potentially increase open source community interest. But i fear that oui: will give people second-thought (ie: are these really tailwind classes? Did OUI do something unique to it? If so, where are the docs? ETC)

But if we use tw: prefix, there is less ambiguity i think

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i also want the components we build to be highly customizable (style-wise) by consumers if needed, and i'm glad that this PR implemented className.

But i think we should use https://www.npmjs.com/package/tailwind-merge for all of our OUI components. This will allow us to safely override classes

<div
data-slot="data-table-column-header"
className={cn('oui:flex oui:items-center oui:space-x-2', className)}
>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think we have been using data-test-subj and an optional data-test-subj prop to be able to target our components in tests. Is that a pattern we want to continue supporting with this new OUI version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants