carbon-design-system / carbon

A design system built by IBM
https://www.carbondesignsystem.com
Apache License 2.0
7.81k stars 1.8k forks source link

[Bug]: DataTable with Expansion does not render data with the expanded column #16018

Closed a-roberts closed 6 months ago

a-roberts commented 6 months ago

Package

@carbon/react

Browser

Chrome

Package version

11.26.0

React version

17.0.2

Description

Hi, I'm looking to move off Carbon 10 and carbon-components-react in favour of Carbon 11 and @carbon/react.

I'm using TypeScript. I don't see any examples I can base off of that are in TypeScript, so I've been bumbling around adding plenty of TS ignores that I perhaps shouldn't, but at least things will render.

The errors in question that I'm TS ignoring are in the area of

Type '({ headers, getHeaderProps }: DataTableProps) => JSX.Element' is not assignable to type '(renderProps: DataTableRenderProps<TablesRowType, (string | AdditionalProps | undefined)[]>) => ReactElement<any, string | JSXElementConstructor<...>>'.
  Types of parameters '__0' and 'renderProps' are incompatible.
    Type 'DataTableRenderProps<TablesRowType, (string | AdditionalProps | undefined)[]>' is not assignable to type 'DataTableProps'.
      Types of property 'headers' are incompatible.
        Type 'DataTableHeader[]' is not assignable to type 'HeaderType[]'.
          Type 'DataTableHeader' is not assignable to type 'HeaderType'.
            Types of property 'header' are incompatible.
              Type 'ReactNode' is not assignable to type 'string'.
                Type 'undefined' is not assignable to type 'string'.

and then

Type '({ rows, headers, getRowProps, getHeaderProps }: DataTableProps) => JSX.Element' is not assignable to type '(renderProps: DataTableRenderProps<TablesRowType, (string | AdditionalProps | undefined)[]>) => ReactElement<any, string | JSXElementConstructor<...>>'.
  Types of parameters '__0' and 'renderProps' are incompatible.
    Type 'DataTableRenderProps<TablesRowType, (string | AdditionalProps | undefined)[]>' is not assignable to type 'DataTableProps'.

this appears in DataTable.d.ts.

I have these version references...

 "@carbon/charts": "^1.15.3",
    "@carbon/charts-react": "^1.15.3",
    "@carbon/colors": "^11.21.0",
    "@carbon/grid": "^11.22.0",
    "@carbon/ibm-products": "^2.31.0",
    "@carbon/icons-react": "^11.38.0",
    "@carbon/layout": "^11.21.0",
    "@carbon/react": "^1.52.0",
    "@carbon/type": "^11.26.0",

I'd love to be able to migrate to carbon-react (on Carbon 11), and this is my final hurdle...hoping folks can help.

Reproduction/example

Provided screenshots

Steps to reproduce

If we're using carbon-components-react, things look like this - no problem:

image

If I use carbon/react only, it looks like this:

image

If I add an extra column, with carbon-react only, it looks like this

image

everything seems oddly...shifted, and I can't figure out why yet.

We used to use colSpan but that doesn't exist on TableExpandRow, but it did when we were using carbon-components-react, so that could be a clue.

My current thoughts involve taking a look into the DataTable source code more and building this up from scratch (or rewriting this in JS which I'm trying to avoid, but I think I'd have the same problem).

What I'm ultimately looking for is a TypeScript example using expandable rows of a DataTable please.

I'll come back to this tomorrow and provide an easy peasy reproduce that's all self-contained since what I'm working with contains a whole bunch of variables and it's not the easiest to read given it's all super dynamic...

I'd also like to add:

Type '{ children: (Element | undefined)[]; isExpanded: false; "data-testid": string; key: string; }' is missing the following properties from type 'TableExpandRowProps': ['aria-label'], onExpand

I don't know what to implement under onExpand (yes, I can have it refer to an empty React.MouseEvent of sorts, but that doesn't feel right).

Any help would be brilliant - I'm hoping for simple user error and that's why I'll work on an easy peasy small test case and share that, tomorrow - cheers

Suggested Severity

Severity 2 = User cannot complete task, and/or no workaround within the user experience of a given component.

Application/PAL

No response

Code of Conduct

a-roberts commented 6 months ago

https://github.com/a-roberts/carbon-datatable-expansion-ts-bug/tree/main

I've (well, attempted) a minimal reproduce here that's as small as possible although this is even further behind in the styling department now as I'm not sure which styles to be importing or if I've configured "dart scss" correctly...although if I take out those mixims, things look even worse, so they must be taking some effect.

I'll do whatever's needed next to make it look as I posted in the original comment and report back, but at the same repo I've mentioned why I've got ignores in the place I do, and I've gone with the scss I have.

image

for convenience, this is my progress so far in the minimal TypeScript reproduce case. It's not good, I am struggling to figure it out based on looking at https://react.carbondesignsystem.com/?path=/docs/components-datatable-expansion--overview and the storybook code.

a-roberts commented 6 months ago

Ah, didn't realise the stylesheets are cumulative

image
@use '@carbon/react/scss/reset';
@use '@carbon/react/scss/type' as *;
@use '@carbon/react/scss/theme' as *;
@use '@carbon/styles/scss/components/data-table';
@use '@carbon/styles/scss/components/data-table/expandable';

so I think this is the reproduce, and my GitHub repo is updated.

a-roberts commented 6 months ago
image

I've "rewritten" (barely) this in JavaScript. Same problem.

Pushed to the same repo https://github.com/a-roberts/carbon-datatable-expansion-ts-bug/tree/main

tw15egan commented 6 months ago

@a-roberts seems like you might be missing the TableExpandHeader before the map over the headers. I believe that is needed so that the headers count match

<TableHead>
              <TableRow>
                <TableExpandHeader aria-label="expand row" />
                {headers.map((header) => (
                  // @ts-expect-error complains about MouseEvent missing properties from type MouseEvent. What mouse event?
                  <TableHeader {...getHeaderProps({ header })}>
                    {header.header}
                  </TableHeader>
                ))}
              </TableRow>
            </TableHead>

https://stackblitz.com/edit/github-wxa3ax?file=src%2FApp.jsx

a-roberts commented 6 months ago

https://stackblitz.com/edit/github-wxa3ax?file=src%2FApp.jsx

image

Thank you, and now I feel silly.

That works and I've pushed to my repo. I'll have to figure out the other type errors and I'll see if this fixes my "non-minimised" test case or if we're good to close this. Much appreciated

a-roberts commented 6 months ago

And fixed in the non-minified case too via the use of <TableExpandHeader aria-label='expand row' />

hurray, thank you