[docs] Improve sorting documentation page #3564

flaviendelangle · 2022-01-07T14:32:54Z

Doc preview

Closes #1511

Package changes

Expose the built-in comparator to let the user use them when building custom comparator. They check for nullish value for instance, most of the time I don't think users should start a comparator from scratch.

Improve demos

Improve the custom comparator demo to display a fully-custom comparator and a custom comparator based on the build-in ones
Add demos for the initialization and the control of the model
Always use the same dataset (I used the same as filtering, maybe we could make this basic dataset more universal and use it on every demo where we care about the columns content and not the columns amount or sizing,
Only use controlled mode when it's the goal of the demo, otherwise use initialState

Improve paragraph order

Before:

After:

I have a few doubts on the titles of the sections
I tried to keep the order as close as the sorting one as possible to have a consistency.

mui-bot · 2022-01-18T09:53:36Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	180.1	504.7	341.6	332.74	108.92
Sort 100k rows	ms	507.8	769.4	651.3	643.24	83.87
Select 100k rows	ms	223.7	294.2	241.7	251.76	25.601
Deselect 100k rows	ms	122.6	290.3	221.5	219.08	63.169

Generated by 🚫 dangerJS against 8610917

github-actions · 2022-01-19T09:36:12Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

alexfauquette

Seem realy nice. I only read the markdown. I re-request my review to read example later

alexfauquette · 2022-01-19T12:13:56Z

docs/src/pages/components/data-grid/sorting/sorting.md


-A sorted column can be can pre-configured using the `sortModel` prop:
+## Single and multi-sorting


This section content is only about multi-sorting

Suggested change

## Single and multi-sorting

## Single and multi-sorting [<span class="plan-pro"></span>](https://mui.com/store/items/material-ui-pro/)

https://deploy-preview-3437--material-ui-x.netlify.app/components/data-grid/filtering/#single-and-multi-filtering

We did not put the pro label on the filtering because the section also contains the mention that the DataGrid does not handle multi filtering / multi sorting.

That's a good point, @flaviendelangle. But I've just recently reviewed the license page, and because we mention there we'd add the label on pro (and premium) features, I agree we should consider adding the 'pro' label here.
Could we also consider making a patch for the filtering page regarding the changes we discuss here?

Having that said, I like the highlight box stating an important difference between the free and pro versions.
Perhaps we can leave this section as is and add a specific labeled section for the multi-sorting just after it.

I'm in favor of consistency
But if the best output is with the pro icon, then there is no problem adding it to the filter page at the same time 👍

Perhaps we can leave this section as is and add a specific labeled section for the multi-sorting just after it.

I added it to see based on https://github.com/mui-org/material-ui-x/pull/3564/files#r791072821 to see what it would look like.
If we agree it's better that way, I'll do the same for filtering.

Thanks. I think the preview is looking great.

alexfauquette · 2022-01-19T12:17:31Z

docs/src/pages/components/data-grid/sorting/sorting.md

+
+{{"demo": "pages/components/data-grid/sorting/ControlledSort.js", "bg": "inline", "defaultCodeOpen": false}}
+
+## Disable the sorting


I think this section could be introduced before multi sorting because: It does not need to know the existence of multi-sorting, and it is simpler to understand than sorting model

Would you have the same logic for filtering ?
I kept the same order between the two pages as the logic are similar.

I don't know.

Having the same structure is clearly a good point (I was wondering why the flow seems familiar 😅)

I noticed it for this page (and not the filtering one) because here goes from a section with 3 subsections to a small section with one sentence + an example.

Let see if @joserodolfofreitas and @cherniavskii have an opinion

As I understand both single and multi-sorting works out of the box without any explicit configuration.
it can't be easier than that ;)

Maybe we should add a comment on the demo code that the configuration is optional, or remove the configuration altogether. Although I understand it's useful to have the demo showing by default the sorted data, so we have to weigh on that too.

Another point I see: it's good to highlight this hot pro feature (single vs multi) on the beginning of the page.

Not sure to follow you here

or remove the configuration altogether

What configuration are we talking about ?

You're right, there's no configuration on the single sorting and multi sorting basic demos (great!). I may have mixed it with another one that sets a default sorting rule (which was the config I was referring to). My bad!

You can disable the multi sorting on the pro
But I think this option mainly exist to limit on the free, we don't even document it 👍

docs/src/pages/components/data-grid/sorting/sorting.md

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

docs/src/pages/components/data-grid/sorting/sorting.md

docs/src/pages/components/data-grid/sorting/OrderSortingPerColumnGrid.tsx

docs/src/pages/components/data-grid/sorting/ComparatorSortingGrid.tsx

docs/src/pages/components/data-grid/sorting/BasicExampleDataGrid.tsx

docs/src/pages/components/data-grid/sorting/sorting.md

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

…id.tsx Co-authored-by: Matheus Wichman <matheushw@outlook.com>

cherniavskii · 2022-01-20T16:15:43Z

docs/src/pages/components/data-grid/sorting/BasicExampleDataGrid.tsx

+
+const VISIBLE_FIELDS = ['name', 'rating', 'country', 'dateCreated', 'isAdmin'];
+
+export default function BasicExampleDataGrid() {


I've noticed that country sorting doesn't work in this demo

Screen.Recording.2022-01-20.at.17.05.04.mov

I can reproduce it in storybook as well.

It seemed like an easy fix at first - just a matter of adding valueGetter, so that column gets sorted by country label.
It didn't work though.
Adding valueGetter broke renderCell, since renderCell now receives a string (returned by valueGetter) instead of whole field object.
I did not expect that and at this point I'm not sure if that's intended.

I can extract it into a separate issue and investigate, so that this PR is not blocked by it.

docs/src/pages/components/data-grid/sorting/sorting.md

joserodolfofreitas · 2022-01-24T17:20:01Z

docs/src/pages/components/data-grid/sorting/sorting.md


-A sorted column can be can pre-configured using the `sortModel` prop:
+## Single and multi-sorting


That's a good point, @flaviendelangle. But I've just recently reviewed the license page, and because we mention there we'd add the label on pro (and premium) features, I agree we should consider adding the 'pro' label here.
Could we also consider making a patch for the filtering page regarding the changes we discuss here?

Having that said, I like the highlight box stating an important difference between the free and pro versions.
Perhaps we can leave this section as is and add a specific labeled section for the multi-sorting just after it.

docs/src/pages/components/data-grid/sorting/sorting.md

joserodolfofreitas

It's looking great!
I added just a small fix I missed on the "single vs multi" highlight box

docs/src/pages/components/data-grid/sorting/sorting.md

Co-authored-by: José Rodolfo Freitas <joserodolfo.freitas@gmail.com>

[docs] Improve sorting documentation page

7cb0172

flaviendelangle added the docs Improvements or additions to the documentation label Jan 7, 2022

flaviendelangle self-assigned this Jan 7, 2022

flaviendelangle added 3 commits January 7, 2022 15:36

Work

9b0f2e4

Merge branch 'master' into sorting-doc

9e3806d

Work

6c9e97e

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

flaviendelangle added 2 commits January 19, 2022 10:46

Merge branch 'master' into sorting-doc

eb882dc

Merge

0ce83bc

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

Work

870f01e

flaviendelangle marked this pull request as ready for review January 19, 2022 10:21

flaviendelangle requested review from cherniavskii, alexfauquette and joserodolfofreitas January 19, 2022 10:21

alexfauquette reviewed Jan 19, 2022

View reviewed changes

alexfauquette self-requested a review January 19, 2022 12:48

flaviendelangle and others added 7 commits January 19, 2022 16:51

Update docs/src/pages/components/data-grid/sorting/sorting.md

aeb7978

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

cfbff82

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

d865686

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

f65b3ef

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

5358761

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

cb42ba7

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

6a0ec5a

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

alexfauquette reviewed Jan 20, 2022

View reviewed changes

m4theushw reviewed Jan 20, 2022

View reviewed changes

flaviendelangle and others added 2 commits January 20, 2022 15:40

Update docs/src/pages/components/data-grid/sorting/sorting.md

56a764a

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

99d4b45

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

flaviendelangle and others added 3 commits January 20, 2022 15:40

Update docs/src/pages/components/data-grid/sorting/sorting.md

a13c66f

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

Update docs/src/pages/components/data-grid/sorting/sorting.md

4c4d952

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

Update docs/src/pages/components/data-grid/sorting/BasicExampleDataGr…

512287d

…id.tsx Co-authored-by: Matheus Wichman <matheushw@outlook.com>

alexfauquette mentioned this pull request Jan 20, 2022

[core] Add eslint rule to force default export equals to filename in doc #3674

Merged

cherniavskii reviewed Jan 20, 2022

View reviewed changes

cherniavskii mentioned this pull request Jan 20, 2022

[Docs] Country sorting doesn't work on demo page #3681

Closed

flaviendelangle added 2 commits January 21, 2022 10:06

Code review: Alexandre

6a6f0cf

Code review: Alexandre

863edf9

flaviendelangle requested review from alexfauquette and m4theushw January 21, 2022 09:09

Empty

5fe9fd8

joserodolfofreitas reviewed Jan 24, 2022

View reviewed changes

Code review: José

5db57dc

flaviendelangle requested a review from joserodolfofreitas January 25, 2022 09:07

Merge branch 'master' into sorting-doc

fcf6ede

joserodolfofreitas approved these changes Jan 25, 2022

View reviewed changes

docs/src/pages/components/data-grid/sorting/sorting.md Outdated Show resolved Hide resolved

Update docs/src/pages/components/data-grid/sorting/sorting.md

8610917

Co-authored-by: José Rodolfo Freitas <joserodolfo.freitas@gmail.com>

cherniavskii mentioned this pull request Jan 26, 2022

[docs] Fix country column sorting not working #3740

Merged

flaviendelangle merged commit 6be6e0d into mui:master Jan 26, 2022

flaviendelangle deleted the sorting-doc branch January 26, 2022 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Improve sorting documentation page #3564

[docs] Improve sorting documentation page #3564

flaviendelangle commented Jan 7, 2022 •

edited

Loading

mui-bot commented Jan 18, 2022 •

edited

Loading

github-actions bot commented Jan 19, 2022

alexfauquette left a comment

alexfauquette Jan 19, 2022

flaviendelangle Jan 19, 2022

joserodolfofreitas Jan 24, 2022

flaviendelangle Jan 25, 2022

joserodolfofreitas Jan 25, 2022

alexfauquette Jan 19, 2022

flaviendelangle Jan 19, 2022

alexfauquette Jan 19, 2022

joserodolfofreitas Jan 24, 2022

flaviendelangle Jan 26, 2022

joserodolfofreitas Jan 26, 2022 •

edited

Loading

flaviendelangle Jan 26, 2022

cherniavskii Jan 20, 2022

cherniavskii Jan 20, 2022

joserodolfofreitas Jan 24, 2022

joserodolfofreitas left a comment


		A sorted column can be can pre-configured using the `sortModel` prop:
		## Single and multi-sorting

	## Single and multi-sorting
	## Single and multi-sorting [<span class="plan-pro"></span>](https://mui.com/store/items/material-ui-pro/)


		{{"demo": "pages/components/data-grid/sorting/ControlledSort.js", "bg": "inline", "defaultCodeOpen": false}}

		## Disable the sorting


		const VISIBLE_FIELDS = ['name', 'rating', 'country', 'dateCreated', 'isAdmin'];

		export default function BasicExampleDataGrid() {

[docs] Improve sorting documentation page #3564

[docs] Improve sorting documentation page #3564

Conversation

flaviendelangle commented Jan 7, 2022 • edited Loading

Package changes

Improve demos

Improve paragraph order

mui-bot commented Jan 18, 2022 • edited Loading

github-actions bot commented Jan 19, 2022

alexfauquette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joserodolfofreitas Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joserodolfofreitas left a comment

Choose a reason for hiding this comment

flaviendelangle commented Jan 7, 2022 •

edited

Loading

mui-bot commented Jan 18, 2022 •

edited

Loading

joserodolfofreitas Jan 26, 2022 •

edited

Loading