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

[DataGrid] Column spanning #4020

Merged
merged 42 commits into from
Apr 12, 2022
Merged

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Feb 23, 2022

Closes #192

Docs: https://deploy-preview-4020--material-ui-x.netlify.app/x/react-data-grid/columns/#column-spanning
Storybook demo: https://deploy-preview-4020--material-ui-x.netlify.app/storybook/?path=/story/datagridpro-test-columns--column-spanning

Changelog

TODO:

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request feature: Rendering layout Related to the data grid Rendering engine labels Feb 23, 2022
@mui-bot
Copy link

mui-bot commented Feb 23, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 247 655.8 344.5 420.72 174.566
Sort 100k rows ms 490.3 985.6 797.5 741.26 208.086
Select 100k rows ms 148 199.6 181.6 176.58 17.766
Deselect 100k rows ms 102.9 292.4 149.2 174.64 67.802

Generated by 🚫 dangerJS against 919cffb

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

This comment was marked as resolved.

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

This comment was marked as outdated.

@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 Feb 25, 2022
@github-actions

This comment was marked as outdated.

@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 Mar 2, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 4, 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 Mar 4, 2022
@m4theushw m4theushw self-requested a review March 7, 2022 15:52
@cherniavskii
Copy link
Member Author

Check if this can be solved by columnThreshold={0}.

Changing columnThreshold doesn't help, but disabling column virtualization solves the issue (columnBuffer={columns.length}).
Not sure if it is acceptable solution for now

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

This comment was marked as outdated.

@m4theushw
Copy link
Member

m4theushw commented Mar 8, 2022

I've one proposal to add support for column virtualization.

First, open https://codesandbox.io/s/columnspanning-material-demo-forked-zcu0n0 and scroll horizontally 201px:

document.querySelector('.MuiDataGrid-virtualScroller').scrollLeft = 201

Since the Quantity column is not fully visible, renderContext.firstColumnIndex=2 and firstColumnToRender=1 (I modified the buffer). In this position, only the non-spanned Quantity cells are rendered. After the last item, we should see a yellow background from the spanned Subtotal but we don't see it. It's not rendered because, in the current implementation, the first cell of a spanned column takes the width necessary for the other cells and if that cell is not rendered the width is not taken.

image

To solve that we can render the first cell of the spanned column. The proposal is basically to modify firstColumnToRender in a way where if its value touches a cell from a spanned column we decrease it until no cell in the current firstColumnToRender is coming from a spanned column.

In pseudo-code:

firstColumnToRender = getFirstColumnToRender(scrollLeft, columnBuffer) // we already have this

while firstColumnToRender > 0 and willTouchSpannedColumn(firstColumnToRender, scrollLeft)
  firstColumnToRender--

function willTouchSpannedColumn(columnIndex, scrollLeft)
  for i=firstRowToRender to lastRowToRender
    // gets the left position of the cell at x=columnIndex and y=i
    // needs to consider that it may be spanned by other columns
    left=columnPositions[columnIndex - getSpannedBy(columnIndex, i)]
    if left < scrollLeft
      return true
  return false

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 8, 2022
@m4theushw m4theushw removed their request for review March 11, 2022 13:41
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 15, 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 Mar 16, 2022
@cherniavskii cherniavskii requested a review from m4theushw April 5, 2022 15:53
@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 Apr 9, 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.

Nice job with the feature!
I loved the example exploring the function signature with the week calendar.

Leaving just a couple of minor tweaks on the docs page.

docs/data/data-grid/columns/columns.md Outdated Show resolved Hide resolved
docs/data/data-grid/columns/columns.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 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.

I think it's good enough to be released. Later https://mui.com/x/react-data-grid/group-pivot/#row-grouping can use column spanning in the grouping columns.

@cherniavskii
Copy link
Member Author

@m4theushw

Later mui.com/x/react-data-grid/group-pivot/#row-grouping can use column spanning in the grouping columns.

Where exactly column spanning may be useful?

@cherniavskii cherniavskii merged commit b88ecff into mui:master Apr 12, 2022
@cherniavskii cherniavskii deleted the column-spanning branch April 12, 2022 09:53
@m4theushw
Copy link
Member

Where exactly column spanning may be useful?

The grouping column could span the entire row. Currently, we cut the text.

image

@flaviendelangle
Copy link
Member

With aggregation the other cells can contain data (even without aggregation, users can populate them if they want, but it's probably not very frequent)

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: Rendering layout Related to the data grid Rendering engine new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Implement Column spanning
6 participants