Skip to content

Conversation

@romgrk
Copy link
Contributor

@romgrk romgrk commented May 30, 2024

@romgrk romgrk added scope: data grid Changes related to the data grid. feature: Row pinning Related to the data grid Row pinning feature labels May 30, 2024
@mui-bot
Copy link

mui-bot commented May 30, 2024

Deploy preview: https://deploy-preview-13313--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against e1a1337

@romgrk romgrk enabled auto-merge (squash) June 18, 2024 04:36
@romgrk romgrk disabled auto-merge June 18, 2024 04:41
Copy link
Member

@cherniavskii cherniavskii 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!
Can you add a unit test for this regression?

@cherniavskii cherniavskii added plan: Pro Impact at least one Pro user. type: regression A bug, but worse, it used to behave as expected. labels Jun 18, 2024
@cherniavskii cherniavskii changed the title [DataGrid] Keep bottom pinned row at the bottom [DataGridPro] Keep bottom pinned row at the bottom Jun 18, 2024
Comment on lines -516 to -524
// In cases where the columns exceed the available width,
// the horizontal scrollbar should be shown even when there're no rows.
// Keeping 1px as minimum height ensures that the scrollbar will visible if necessary.
const height = Math.max(contentHeight, 1);

Copy link
Contributor Author

@romgrk romgrk Jun 19, 2024

Choose a reason for hiding this comment

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

I think this code was preventing this 0-height warning from running in some cases, so fixing it broke some tests all over the place that didn't have explicit dimensions on the grid container. I'll need to debug & update those tests before merging this PR. The test runner also doesn't seem to match the warning to which test it originated from precisely, so sometimes it marks one test as failed but the warning came from another test. As in if I run the test with .only it succeeds, but if I run the test suite as a whole it fails. Lots of fun ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: Row pinning Related to the data grid Row pinning feature plan: Pro Impact at least one Pro user. scope: data grid Changes related to the data grid. type: regression A bug, but worse, it used to behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[data grid] The bottom pinned row in a data grid does not stay at the bottom when there aren't any other rows in the data set

5 participants