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] Remove GridCell borderBottom when last row #3519

Merged

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Dec 28, 2021

Fixes #2315

I've done a potential fix for the issue. The drawback is that now that you have fewer rows than what the grid can load (a black space is visible) and you have a footer enabled then the footer also has a borderTop. I think this is how it should have been from the beginning but still, it is technically a breaking change so we need to review it before proceeding.

You can check what the change is here: ...

https://deploy-preview-3519--material-ui-x.netlify.app/components/data-grid/

@DanailH DanailH added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Dec 28, 2021
@DanailH DanailH self-assigned this Dec 28, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 30, 2021

It reminds me of this attempt that I did in #271 at the problem but on which I failed by lack of time. IMHO the right solution is trickier. I see a couple of KO cases in the live preview.

for (let i = 0; i < renderedRows.length; i += 1) {
const { id, model } = renderedRows[i];

const isLastRow = i === renderedRows.length - 1;
Copy link
Member

Choose a reason for hiding this comment

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

This will be true when on the last row of the current render context, which is different from the index relative to all visible rows. Is this what you want? The index prop gives the "real" index.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the index might also work. The thing is that I need to know if there are fewer rows than the height of the viewport so I can add the border to the last row.

Copy link
Member

Choose a reason for hiding this comment

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

I think what you need to do is something that's already being done for the columns. Instead of columnsMeta.totalWidth, use currentPage.rows.length * rowHeight. To get the height you can try using apiRef.current.getRootDimensions.

https://github.com/mui-org/material-ui-x/blob/6208886dbb143f7365f8387ef9d91a6261a3a9d3/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualScroller.tsx#L299-L303

Copy link
Member Author

Choose a reason for hiding this comment

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

But the current way also works. I don't need to calculate the height, I can know if there is space just by checking the indexes.

@DanailH
Copy link
Member Author

DanailH commented Jan 4, 2022

It reminds me of this attempt that I did in #271 at the problem but on which I failed by lack of time. IMHO the right solution is trickier. I see a couple of KO cases in the live preview.

Yes, I saw the autoHeight problem. Besides that and the fact that now if there are fewer rows than what the viewport can take (you have an empty space between the footer and the last row) then both the last row and the footer have a border.

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

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 2, 2022
@DanailH DanailH requested a review from m4theushw February 2, 2022 15:57
@mui-bot
Copy link

mui-bot commented Feb 2, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 282.1 679 408.2 432.14 138.971
Sort 100k rows ms 564.6 1,234 1,234 915.56 228.839
Select 100k rows ms 185.2 279.7 216.7 232.38 35.886
Deselect 100k rows ms 102.6 203.4 175.8 164.34 37.608

Generated by 🚫 dangerJS against c1e6fce

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

github-actions bot commented Feb 3, 2022

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

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

siriwatknp commented Feb 8, 2022

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/data-grid/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/x/api/data-grid/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

@DanailH DanailH requested a review from m4theushw February 17, 2022 11:31
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 17, 2022
@github-actions
Copy link

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 23, 2022
@DanailH DanailH requested a review from m4theushw February 23, 2022 12:52
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 1, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 3, 2022
@DanailH DanailH requested a review from m4theushw March 3, 2022 11:14
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.

Good job!

@DanailH DanailH merged commit 242d109 into mui:master Mar 3, 2022
@DanailH DanailH mentioned this pull request Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] autoHeight + hideFooter results in double bottom border
5 participants