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

[docs] Improve the getRowId doc section #5156

Merged
merged 5 commits into from
Jun 14, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions docs/data/data-grid/rows/rows.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,34 @@ Otherwise, the grid will re-apply heavy work like sorting and filtering.

{{"demo": "RowsGrid.js", "bg": "inline"}}

:::warning
Each row object should have a field that uniquely identifies the row.
By default, the grid will use the `id` property of the row. Note that [column definition](/x/react-data-grid/column-definition/) for `id` field is not required.
## Row identifier

Each row must have a unique identifier.

This identifier is used internally to identify the row in the various models—for instance, the row selection model—and to track the row across updates.

When using dataset without a unique `id` property, you can use the `getRowId` prop to specify a custom id for each row.
By default, the data grid looks for a property named `id` in the data set to get that identifier.

If the row's identifier is not called `id`, then you need to use the `getRowId` prop to tell the grid where it's located.

The following demo shows how to use `getRowId` to grab the unique identifier from a property named `internalId`:
Comment on lines +20 to +30
Copy link
Member

@oliviertassinari oliviertassinari Jun 14, 2022

Choose a reason for hiding this comment

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

Are we sure about the number of line breaks? It feels challenging to skim read, there are multiple paragraphs that feel talk about almost the same topic:

Screenshot 2022-06-14 at 13 31 05

I would personally enjoy reading this denser version (1. what it is, 2. behavior/configuration 3. a demo):

Screenshot 2022-06-14 at 13 10 58

Copy link
Member Author

Choose a reason for hiding this comment

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

We can improve that one in #5195
I felt like it was already an improvement and could be shiped as is if needed

Copy link
Member

@oliviertassinari oliviertassinari Jun 14, 2022

Choose a reason for hiding this comment

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

I felt like it was already an improvement and could be shiped as is if needed

@flaviendelangle For sure 👍 . This is only an instance of a larger topic.

My root question is about: what should be our general practice about paragraphs and what's our threshold to split sentences into multiple paragraphs once the content cover is too further away? It's mostly a question for @samuelsycamore. We started to surface the matter in the past. I feel that our current threshold is too low, I want to advocate for the side that says: we don't group enough. Another instance was mui/material-ui#32325 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Both of these look good to me, so I'm happy to accommodate either style. I would say that in general, I don't think paragraphs should be any longer than they are in the latter version. With dense material like this, it's easy to get lost in the middle of a paragraph.

I think this paragraph is the ideal length for text like this:

Screen Shot 2022-06-14 at 9 11 03 AM

So we could say that as a general rule, you should consider a line break after 2 sentences or ~200 characters.

Copy link
Member

@oliviertassinari oliviertassinari Jun 14, 2022

Choose a reason for hiding this comment

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

cc @mbrookes for perspective on the matter.


If we were to look for rules that could work in practice, I would personally imagine that above 500 characters, a paragraph is either too verbose to cover one matter and should be broken down, or it's actually covering multiple matters and hence should be broken down too. In the docs of Stripe (one I would assume receives a lot of care), it's common to find paragraphs of almost 400 characters e.g. https://stripe.com/docs/products-prices/how-products-and-prices-work but It's hard to find one above 500.

Copy link
Member

Choose a reason for hiding this comment

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

I'm with @oliviertassinari on this one. The second example is superior to the first. But if they needed to be to convey the concept, each paragraph could comfortably be twice as long. I don't think that there should be an arbitrary rule on paragraph length – paragraphs aren't measured in words or sentences; but I agree that verbosity should be eliminated, and that paragraphs should cover one narrow topic.


```tsx
<DataGrid getRowId={(row) => row.internalId} />
```

{{"demo": "RowsGridWithGetRowId.js", "bg": "inline", "defaultCodeOpen": false}}

flaviendelangle marked this conversation as resolved.
Show resolved Hide resolved
If no such unique identifier exists in the data set, then you must create it by some other means, but this scenario should be avoided because it leads to issues with other features of the grid.

Note that it is not necessary to create a column to display the unique identifier data.
The data grid pulls this information directly from the data set itself, not from anything that is displayed on the screen.

:::warning
Just like the `rows` prop, the `getRowId` prop should keep the same reference between two renders.
Otherwise, the grid will re-apply heavy work like sorting and filtering.
:::

## Updating rows

### The `rows` prop
Expand Down