🐛(react) fix datagrid column unique key #119
Conversation
🦋 Changeset detectedLatest commit: 33c3ea4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
AntoLC
added
bug
|
Hi, I am not sure if I should do something about the |
AntoLC
force-pushed
the
issue/datagrid-unique-keys
branch
from
5b06b62
to
1b20ece
Compare
AntoLC
force-pushed
the
issue/datagrid-unique-keys
branch
from
1b20ece
to
3535859
Compare
|
@AntoLC as you are fixing a minor bug, I would add a changeset. |
AntoLC
force-pushed
the
issue/datagrid-unique-keys
branch
from
3535859
to
9a8609d
Compare
| let headlessColumns = columns.map((column) => { | ||
| let headlessColumns = columns.map((column, index) => { | ||
| const opts = { | ||
| id: column.field ?? "actions", | ||
| id: column.field ?? `actions_${index}`, |
There was a problem hiding this comment.
Crystal clear! Since I haven't had the opportunity to work on this particular component, my question may not be directly relevant. Therefore, please feel free to disregard it if it doesn't apply.
I noticed that when reordering the columns, the column's id would be updated. However, I'm curious whether this would also trigger a re-rendering of the column content, even if the content itself hasn't changed. In React, using the index as a key for a list is considered an antipattern. I'm wondering if the same principle applies in this scenario as well.
There was a problem hiding this comment.
Yes it's not perfect, but we don't have too much data to rely on (https://github.com/openfun/cunningham/blob/main/packages/react/src/components/DataGrid/index.tsx#L26-L32), we could eventually improve a bit by doing something like that:
id: column.field ?? `actions-${column.headerName ? `${column.headerName}-${index}` : index }`,
Open to suggestion.
There was a problem hiding this comment.
We could do something like that as well (dd06f1a), an id property is mandatory if the property renderCell is defined. The bad side is that it creates a breaking change with the datagrid with renderCell defined.
NathanVss
force-pushed
the
issue/datagrid-unique-keys
branch
from
dd06f1a
to
849852e
Compare
If we had more than 1 columns with custom cells, we would get a warning about duplicate keys, because every columns got the key `actions`. We add the column index to the key to make it unique.
NathanVss
force-pushed
the
issue/datagrid-unique-keys
branch
from
849852e
to
33c3ea4
Compare
Purpose
If we had more than 1 columns with custom cells, we would get a warning about duplicate keys, because every columns got the key
actions.Proposal
We add the column index to the key to make it unique.