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

[DataGridPro] Implement Row pinning #4863

Merged
merged 50 commits into from
Jul 27, 2022
Merged

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented May 13, 2022

Closes #1251

Preview: https://deploy-preview-4863--material-ui-x.netlify.app/x/react-data-grid/rows/#row-pinning
Row pinning + aggregation: https://deploy-preview-4863--material-ui-x.netlify.app/x/react-data-grid/aggregation-next/

TODO

API

  • option A - current approach

    pinnedRows?: { top?: GridRowModel[]; bottom?: GridRowModel[] };

    Pros:

    • Works with server-side pagination - since pinned rows are not included in the rows prop and provided separately.
    • Works with row grouping - pinned rows will not be included in groups

    Cons:

    • more overhead in simple scenarios. For example, when users want to statically pin some rows from the dataset they have to manipulate the data:
    const rows = [/* ... */];
    const pinnedRowsTop = rows.find(row => row.isPinned);
    const pinnedRows = { top: pinnedRowsTop };
  • option B

    pinnedRows?: { top?: GridRowId[]; bottom?: GridRowId[] };

    Pros:

    • simpler API

    Cons:

    • Doesn't work with server-side pagination

Layout

Row pinning should work with column pinning, so I've implemented the following layout:

Untitled-2022-05-13-1251

@mui-bot
Copy link

mui-bot commented May 13, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 273 488.9 379.1 367.58 84.645
Sort 100k rows ms 541.4 1,087.5 541.4 839.38 202.775
Select 100k rows ms 195.3 336 221.2 236.5 50.897
Deselect 100k rows ms 136.2 314.9 153.4 206.1 74.106

Generated by 🚫 dangerJS against 66ae329

@cherniavskii cherniavskii changed the title [DataGridPro] Row pinning [DataGridPro] Row pinning (WIP) May 16, 2022
@flaviendelangle

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 May 23, 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 7, 2022
@cherniavskii cherniavskii force-pushed the row-pinning branch 4 times, most recently from ff42b31 to 20bb8cb Compare June 20, 2022 10:36
@cherniavskii cherniavskii force-pushed the row-pinning branch 2 times, most recently from 8e2534e to 6e551c0 Compare June 24, 2022 14:12
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 29, 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 Jul 4, 2022
@cherniavskii cherniavskii force-pushed the row-pinning branch 5 times, most recently from 8ad9d91 to 9d1b57c Compare July 11, 2022 07:40
@cherniavskii cherniavskii marked this pull request as ready for review July 11, 2022 08:31
@cherniavskii cherniavskii changed the title [DataGridPro] Row pinning (WIP) [DataGridPro] Implement Row pinning Jul 11, 2022
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Looks good! A topic for discussion. Should we sum the pinned rows when displaying the total row count in the footer? Technically they are rows.

image

@cherniavskii
Copy link
Member Author

cherniavskii commented Jul 26, 2022

@m4theushw

Should we sum the pinned rows when displaying the total row count in the footer? Technically they are rows.

I'm not sure, there's few things to consider:

  1. It depends on the data you put into pinned rows. If pinned rows are of the same type as rest of the rows - then it would make sense to include them in total row count, but If pinned rows are more like headers/footers with additional info - not necessarily.
  2. Pagination - consider 18 rows + 2 pinned rows with a page size of 5. Since pinned rows are visible on each page, it's hard to represent rows count that won't be misleading, for example:
    • page 1: 1–7 of 20 - rows 1-5 + 2 pinned
    • page 2: 6-12 of 20 - rows 6-10 + 2 pinned
    • page 3: 11-17 of 20 - rows 11-15 + 2 pinned
    • page 4: 16-20 of 20 - rows 16-18 + 2 pinned
      Also, it's impossible to achieve behavior like this using TablePagination component.
      AG grid for example doesn't include pinned rows: https://plnkr.co/edit/XeF6TpAEZ3g7lY7K?open=main.js&preview

I would rather not include pinned rows in the row count. We can reconsider this once we get some feedback on it.
What do you think?

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.

I think the documentation feels a tad too busy now with the multiple callouts.
Perhaps we can spread them along the row pinning documentation.

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 Outdated Show resolved Hide resolved
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.

Again, great work, @cherniavskii!
Glad to see this live soon.

Just a couple last minor suggestions:
We're missing to add the feature at our features list on overview

docs/data/data-grid/rows/rows.md Outdated Show resolved Hide resolved
@m4theushw
Copy link
Member

I would rather not include pinned rows in the row count. We can reconsider this once we get some feedback on it.
What do you think?

No problem in waiting for feedback from the community, since the feature is experimental. About including the pinned rows when pagination is enabled, yeah it would be misleading. But in aria-rowcount I don't know what is the best approach. AG Grid includes the header row in it, we don't. Maybe they should also include header row + pinned rows and we follow the same.

@cherniavskii cherniavskii merged commit e273835 into mui:master Jul 27, 2022
@cherniavskii cherniavskii deleted the row-pinning branch July 27, 2022 16:27
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jul 31, 2022
@oliviertassinari oliviertassinari added plan: Pro Impact at least one Pro user feature: Row pinning Related to the data grid Row pinning feature labels Aug 7, 2022
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! feature: Row pinning Related to the data grid Row pinning feature plan: Pro Impact at least one Pro user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Implement Row pinning
7 participants