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] Split Rows doc page #5195

Merged
merged 29 commits into from
Jul 28, 2022
Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jun 14, 2022

Doc preview

I merged #5156 but we can improve the content here if needed

Some of those pages have outdated content, we can improve them on this PR but if it's heavy changes I would suggest to split and then improve.

I don't really know where to put the "Styling rows" small section.

@m4theushw I moved the Master Detail doc into the "Row" folder instead of "Group & Pivot" as discussed a few weeks ago.

@flaviendelangle flaviendelangle self-assigned this Jun 14, 2022
@flaviendelangle flaviendelangle added the docs Improvements or additions to the documentation label Jun 14, 2022
@mui-bot
Copy link

mui-bot commented Jun 14, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 255.7 651.2 330.4 383.46 143.382
Sort 100k rows ms 485.9 902.2 875.1 800.2 157.964
Select 100k rows ms 144.8 285.2 212.1 215.06 48.163
Deselect 100k rows ms 149.7 255.3 228.7 208.38 39.856

Generated by 🚫 dangerJS against ca150d4


Row reordering allows to rearrange rows by dragging the special reordering cell.

By default, row reordering is disabled.
Copy link
Member Author

Choose a reason for hiding this comment

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

I would add a paragraph describing the default behavior with a link to the Sorting page to let people know how those two features interplay.

I think this information is worth displaying up-front instead of in a warning at the end of the page

docs/data/data-grid/row-ordering/row-ordering.md Outdated Show resolved Hide resolved
/>
```

This approach can also be used to change the location of the toggle column.
Copy link
Member Author

Choose a reason for hiding this comment

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

@DanailH What do you think about copying the structure of this example ?

To display both changes instead of adding an explanation after.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I was thinking in terms that this is something that a user can do so I wasn't sure what would be the benefit of showcasing this, but we can add a demo with the dragged column be the last one. The problem with this is that if you have a horizontal scroll then the dragged column will be out of view.

docs/data/data-grid/row-dimensions/row-dimensions.md Outdated Show resolved Hide resolved
title: Data Grid - Row updates
---

# Data grid - Row updates
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 page will require a lot more content in v6 with #4927 and with all the lazy-loading related topics that @DanailH is handling.

It could even require a dedicated page folder at some point with a page for each approach.
But for now, we can improve this one.

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to merge this page with "row definition", to keep consistency with https://mui.com/x/react-data-grid/column-definition/#providing-content? In essence, we're talking here about different approachs to feed the grid with rows. One of the approachs also allows to update rows (updateRows), but it could also be understood as replacing one row with another (updated) row, so "providing rows via API".

Copy link
Member Author

Choose a reason for hiding this comment

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

@joserodolfofreitas @samuelsycamore I'm interested in your opinion here

I agree that this section could be on the same page as the row update sections

image

But having the "Row identifier" section in the same page as well will start to make the page very long.

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to split that page into 2, one for Row updates and one for Row loading? The Row loading one will have the infinite + lazy loading and anything else related to loading rows on the fly.

Copy link
Member

@joserodolfofreitas joserodolfofreitas Jul 8, 2022

Choose a reason for hiding this comment

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

Yeah, I agree it makes sense to move the lazy loading to its own page.
We could probably use one more depth level on the side bar to hold everything about "feeding data", but I agree it makes sense to separate Row updates from Row Loading (and merge Row definitions with Row updates).

But maybe we could rename these menus?

What about

  • Data binding
  • Lazy loading

?

Copy link
Member

Choose a reason for hiding this comment

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

It can work but calling the page that is about loading rows "Lazy loading" won't be ideal as my idea was to have lazy and infinite loading there.

Copy link
Member

@joserodolfofreitas joserodolfofreitas Jul 14, 2022

Choose a reason for hiding this comment

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

Then:

  • Data binding
  • Lazy and infinite loading

?

Copy link
Member

Choose a reason for hiding this comment

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

@joserodolfofreitas I would add items to the left sidebar. Keeping "lazy loading" inside https://deploy-preview-5195--material-ui-x.netlify.app/x/react-data-grid/row-updates/ will make the page very long. Note that we need to consider that "Row updates" and "Row definition" will be merged.

Copy link
Member

Choose a reason for hiding this comment

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

@DanailH @m4theushw @joserodolfofreitas
Did we reach an agreement on this topic? Or should I create a separate issue for discussion to unlock this PR?

docs/data/data-grid/row-dimensions/row-dimensions.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-dimensions/row-dimensions.md Outdated Show resolved Hide resolved
title: Data Grid - Row updates
---

# Data grid - Row updates
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to merge this page with "row definition", to keep consistency with https://mui.com/x/react-data-grid/column-definition/#providing-content? In essence, we're talking here about different approachs to feed the grid with rows. One of the approachs also allows to update rows (updateRows), but it could also be understood as replacing one row with another (updated) row, so "providing rows via API".

docs/data/pages.ts Outdated Show resolved Hide resolved
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 14, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jun 16, 2022
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 21, 2022
@@ -0,0 +1,54 @@
---
title: Data Grid - Row definition
Copy link
Member

Choose a reason for hiding this comment

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

This feels vague—it isn't clear what to me what it means to "define" a row. It seems like the page is all about defining row identifiers, so I would go with that.

Suggested change
title: Data Grid - Row definition
title: Data Grid - Defining row identifiers

Copy link
Member

Choose a reason for hiding this comment

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

Apart from row identifiers, this page also mentions rows prop structure and has a warning about keeping the same reference.
Also, "Row definition" is consistent with "Column definition" page https://mui.com/x/react-data-grid/column-definition/

Copy link
Member

@cherniavskii cherniavskii Jul 28, 2022

Choose a reason for hiding this comment

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

(Adding missing "Feeding data" section title to make it more clear https://deploy-preview-5195--material-ui-x.netlify.app/x/react-data-grid/row-definition/)

docs/data/data-grid/row-dimensions/row-dimensions.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-dimensions/row-dimensions.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-dimensions/row-dimensions.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-ordering/row-ordering.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-ordering/row-ordering.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-ordering/row-ordering.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-pinning/row-pinning.md Outdated Show resolved Hide resolved
docs/data/data-grid/row-spanning/row-spanning.md Outdated Show resolved Hide resolved
flaviendelangle and others added 5 commits July 5, 2022 09:51
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>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2022
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 27, 2022
@cherniavskii cherniavskii self-assigned this Jul 28, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 28, 2022
Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

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

Regardless of the improvements we may still want to do (like renaming some of the row sub-sections, or reorganizing the content), I think this PR serves its purpose already. It splits the row documentation and it's already a great improvement over what we had before.

@cherniavskii cherniavskii changed the title [docs] Split row doc page [docs] Split Rows doc page Jul 28, 2022
@cherniavskii cherniavskii merged commit ed5d563 into mui:master Jul 28, 2022
@flaviendelangle flaviendelangle deleted the row-doc-split branch August 8, 2022 12:23
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Andrew Cherniavskyi <andrew@mui.com>
@oliviertassinari oliviertassinari mentioned this pull request Oct 10, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants