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

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jun 9, 2022

https://deploy-preview-5156--material-ui-x.netlify.app/x/react-data-grid/rows/#row-identifier

Closes #5126

  • Don't talk about "id field" to avoid confusion between the "id column" and the "id property in the row model".

  • Create a standalone section instead of putting everything in a warning

  • Add a warning saying that getRowId must be memoized

Should we add a note saying that the grid does not require an "id" column ?
I think it's what the Note that [column definition](https://mui.com/x/react-data-grid/column-definition) for id field is not required. tries to say but it's not very comprehensible in its current form.

@flaviendelangle flaviendelangle self-assigned this Jun 9, 2022
@flaviendelangle flaviendelangle added docs Improvements or additions to the documentation component: data grid This is the name of the generic UI component, not the React module! labels Jun 9, 2022
@mui-bot
Copy link

mui-bot commented Jun 9, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 273.3 519.8 346.1 371.9 86.635
Sort 100k rows ms 465 992.5 872.9 775.9 188.354
Select 100k rows ms 136.3 228.6 190.4 185.46 30.009
Deselect 100k rows ms 138.8 268.3 189 189.3 46.24

Generated by 🚫 dangerJS against a510f8d

@flaviendelangle flaviendelangle changed the title [docs] Improve the getRowId doc section [docs] Improve the getRowId doc section Jun 9, 2022
@samuelsycamore
Copy link
Member

samuelsycamore commented Jun 9, 2022

Should we add a note saying that the grid does not require an "id" column ? I think it's what the Note that [column definition](https://mui.com/x/react-data-grid/column-definition) for id field is not required. tries to say but it's not very comprehensible in its current form.

I'm a bit confused by this situation. Tell me if this is correct:

  • Each row must have a unique ID
  • But the grid does not require an id column specifically - the data set may not have unique IDs, or they may be stored in some other property not called id
  • If a data set doesn't have an id field, use the getRowId prop to assign (?) an ID to the row that the grid can use

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Jun 9, 2022

@samuelsycamore

Each row must have a unique ID

Pretty much
To rephrase it, the grid must be able to get a unique id for each row.
Before even talking about where the id is and how the grid knows how to get it, the first thing is that the grid needs a unique id for each row.

But the grid does not require an id column specifically - the data set may not have unique IDs, or they may be stored in some other property not called id

The "id" column, if is exists, has nothing to do with the identifier of the grid. That's a confusion José had and I think he"s not the only one at all.
You can create an column "id" if you want, but the grid will not use it to identify the rows, it will be a regular column to display the id.

If a data set doesn't have an id field, use the getRowId prop to assign (?) an ID to the row that the grid can use

Yes. I just avoid talking about "field" because it's a name tightly linked to the columns (to avoid the confusion described above).
If myRow.id does not contain an unique ID, getRowId allows the dev to say "here is your id for this row".
We can distinguish two main use case I think:

  1. There is a unique identifier in another property of the row model (myRow._id, myRow.index, etc...)

  2. There is no unique identifier and the user must create it (could be by incrementing a global variable, may be by concatenating all the field of the model, ...). This should be avoided because it's can't work well with features like editing or external update of the row, but in some scenario it can work.

@samuelsycamore
Copy link
Member

samuelsycamore commented Jun 9, 2022

Thanks for the clarification @flaviendelangle! It's starting to make more sense to me. The use cases you've described are particularly helpful. My first thought after reading the documentation as it stands was, "what if there is no unique ID in the data set to point to with getRowId?" It sounds like that scenario should be avoided. That might be worth spelling out here.

Here's how I understand it now:

Each row must have a unique identifier.
By default, the grid looks for id in the data set to get that identifier.
If the row's identifier is not called id, you need to use the getRowId to tell the grid where to find that information.
If no such unique identifier exists in the data set, then the developer must create it by some other means, but this scenario should be avoided because it leads to other issues.
In any case, it makes no difference whether or not there is a column in the grid to display the unique identifier data—the grid is pulling directly from the data itself, not from anything that is being displayed.

Is that correct?

As a side note—how do you feel about "unique identifier" here instead of "id" when not referring specifically to id? I think it helps to clarify things.

@flaviendelangle
Copy link
Member Author

Each row much have a unique identifier.
By default, the grid looks for id in the data set to get that identifier.
If the row's identifier is not called id, you need to use the getRowId to tell the grid where to find that information.
If no such unique identifier exists in the data set, then the developer must create it by some other means, but this scenario should be avoided because it leads to other issues.
In any case, it makes no difference whether or not there is a column in the grid to display the unique identifier data—the grid is pulling directly from the data itself, not from anything that is being displayed.

100% correct 👍

As a side note—how do you feel about "unique identifier" here instead of "id" when not referring specifically to id? I think it helps to clarify things.

I think the clearer, the better

Copy link
Member

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

Here's how I'd write this section, now that I understand how it works! 😁

docs/data/data-grid/rows/rows.md Outdated Show resolved Hide resolved
docs/data/data-grid/rows/rows.md Outdated Show resolved Hide resolved
docs/data/data-grid/rows/rows.md Show resolved Hide resolved
flaviendelangle and others added 4 commits June 10, 2022 10:00
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
@flaviendelangle flaviendelangle merged commit 924aecc into mui:master Jun 14, 2022
@flaviendelangle flaviendelangle deleted the row-id-doc branch June 14, 2022 09:46
Comment on lines +20 to +30
## 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`:
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.

alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Explanation around the getRowId is not great
5 participants