Skip to content

Add <DataTable> component #10597

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

Merged
merged 59 commits into from
Apr 7, 2025
Merged

Add <DataTable> component #10597

merged 59 commits into from
Apr 7, 2025

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Mar 18, 2025

Problem

<Datagrid> is fine, but since it uses child inspection, it leads to unexpected bugs when developers wrap children with custom components. For instance, it's not possible to use <CanAccess> to wrap a field:

const PostList = () => (
  <List>
    <Datagrid>
      <TextField source="title" />
      <DateField source="published_at" />
      <CanAccess action="read" resource="posts.views">
        <NumberField source="nb_views" />
      </CanAccess>
   </Datagrid>
 </List>
);

Besides, the signature of all field components is polluted by props that only serve in datagrid columns (e.g. sortBy, label, etc). In fact, field components mix 2 purposes: datagrid columns and record field formatting.

Solution

An alternative data table component named <DataTable> that no longer uses child inspection.

The following works as expected:

const PostList = () => (
  <List>
    <DataTable>
      <DataTable.Col source="title" />
      <DataTable.Col source="published_at" field={DateField} />
      <CanAccess action="read" resource="posts.views">
        <DataTable.Col source="nb_views" field={NumberField} />
      </CanAccess>
   </DataTable>
 </List>
);

The new syntax covers 100% of the <Datagrid> features, and supports various new features.

Pros:

  • IDE-friendly, more discoverable syntax
  • Allows wrapped columns (no WTF)
  • Easier column styling (via <DataTable.Col sx>)
  • Conditional formatting (via <DataTable.Col cellSx>)
  • Built-in support for FunctionField (via <DataTable.Col render>)
  • Support for column selector (no need for <ConfigurableDatagrid>)
  • Optimized by default (no need for optimized prop)
  • More type-safety than Datagrid
  • The sortBy prop is no longer necessary (it's the <DataTable.Col source>)
  • Allows to build custom reusable columns (e.g., <DataTable.NumberCol>)
  • Fields no longer need to support column-only props (sortable, sortByOrder, label, etc).
  • Expandable rows now animate on expand
  • In standalone mode, it's no longer necessary to set bulkActionButtons={false}

Cons:

  • More characters to type
  • In some cases, developers have to repeat the source both in the <DataTable.Col> and in the Field
  • No backward compatibility, the two components will have to live side-by-side for some time

How To Test

I've added a storybook for DataTable and DataTable.Col.

Todos

  • Add <DataTable> component
  • Add <DataTable.Col> component
  • Add <ColumnsButton> component
  • Migrate demos to <DataTable>
  • Add documentation for <DataTable>

Out of cope

  • Make <DataTable> the recommended datagrid component yet (we'll wait for feedback first).
  • Replace every <Datagrid> in documentation with <DataTable>
  • Replace every <Datagrid> in stories with <DataTable>

Additional Checks

  • The PR targets next for a feature
  • The PR includes unit tests (if not possible, describe why)
  • The PR includes one or several stories (if not possible, describe why)
  • The documentation is up to date

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Awesome work! The DX improvement is great. I noticed that only the DataTable component can be targeted in the theme. Is it expected? Also, it would be great to support default props override through theme in all our new components (https://mui.com/material-ui/customization/creating-themed-components/#4-support-theme-default-props).

| `header` | Optional | Element | `<DataTable Header>` | The component rendering the table header. |
| `hiddenColumns` | Optional | Array | `[]` | The list of columns to hide by default. |
| `footer` | Optional | Element | | The component rendering the table footer. |
| `hover` | Optional | Boolean | `true` | Whether to highlight the row under the mouse. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the default value is true, I would instead name this disableHover

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually a MUI TableRow prop, Id rather keep the same syntax.

https://mui.com/material-ui/api/table-row/

```
{% endraw %}

**Tip**: `<DataTable>` already sets `RaDataTable-rowEven` and `RaDataTable-rowOdd` classes on the rows, so you don't need to use a custom DataTable body to implement zebra stripes. You can just use [the `sx` prop](#sx-css-api) to set the background color of these classes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As there is a better way to do this, maybe change the example to add a vertical colored border depending on the status like in our demo review list?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also a better way to do what you describe (cf the rowSx doc). I don't have any other example in mind.

import { type Review } from '../types';

const Column = DataTable.Col<Review>;
const ColumnNumber = DataTable.NumberCol<Review>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one isn't used in the list below

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 175 to 206
<DataTable
rowClick="edit"
hiddenColumns={['total_ex_taxes', 'delivery_fees', 'taxes']}
storeKey={storeKeyByStatus.ordered}
>
<Column source="date">
<DateField source="date" showTime />
</Column>
<Column source="reference" />
<Column source="customer_id" field={CustomerReferenceField} />
<Column label="resources.orders.fields.address">
<ReferenceField
source="customer_id"
reference="customers"
link={false}
>
<AddressField />
</ReferenceField>
</Column>
<ColumnNumber
source="basket.length"
label="resources.orders.fields.nb_items"
/>
<ColumnNumber source="total_ex_taxes" options={currencyStyle} />
<ColumnNumber source="delivery_fees" options={currencyStyle} />
<ColumnNumber source="taxes" options={currencyStyle} />
<ColumnNumber
source="total"
options={currencyStyle}
sx={{ fontWeight: 'bold' }}
/>
</DataTable>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not mistaken, all tables have the same columns except DeliveredOrdersTable that adds the returned column. Can't we have a CommonColumn const that has a Fragment containing the common cols. That's a good test and showcase of what the new DataTable allows too!

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


export const DataTableDataContext = createContext<any[] | undefined>(undefined);

export const useDataTableDataContext = () => useContext(DataTableDataContext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one should accept a RecordType generic parameter and cast the result accordingly

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Co-authored-by: Gildas Garcia <[email protected]>
const Column = DataTable.Col<Order>;
const ColumnNumber = DataTable.NumberCol<Order>;

const OrdersTable = React.memo(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice component but it does not showcase Fragments to group columns that you can still use as the DataTable children which was impossible with Datagrid if I'm not mistaken. Your call though

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Can you:

  • add a i18nProvider to all stories, they would be nicer
  • add more top margin or a title in all stories so that the bulk actions toolbar is not cropped
  • improve the expand stories (the expand content is ugly
  • improve the fullapp story expand content (same)

Also:
In the AccessControl story, when none is selected in the storybook control, you still have the hand mouse pointer which is confusing as there is no action.

Finally, this is awesome!

@djhi djhi mentioned this pull request Apr 4, 2025
@djhi
Copy link
Collaborator

djhi commented Apr 7, 2025

Replying myself on some of these:

  • add a i18nProvider to all stories, they would be nicer

You added it on some stories so I suspect you didn't do it on all on purpose. Ignore this one

Also: In the AccessControl story, when none is selected in the storybook control, you still have the hand mouse pointer which is confusing as there is no action.

I tried to fix it but I can't as we only check for access right on click. Ignore this one

@djhi djhi merged commit 9a27354 into next Apr 7, 2025
16 checks passed
@djhi djhi deleted the datagrid-modern branch April 7, 2025 13:14
@djhi djhi modified the milestones: 5.7.2, 5.8.0 Apr 7, 2025
@fzaninotto fzaninotto mentioned this pull request Apr 14, 2025
4 tasks
@fzaninotto fzaninotto changed the title Add DataTable component Add <DataTable> component Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Development

Successfully merging this pull request may close these issues.

2 participants