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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Fix error when keyboard navigating an empty grid #10081

Merged
merged 4 commits into from
Aug 31, 2023
Merged

[DataGrid] Fix error when keyboard navigating an empty grid #10081

merged 4 commits into from
Aug 31, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Aug 18, 2023 •
edited by MBilalShafi
Loading

Closes #6446

Based on #6535 (comment)

@MBilalShafi I don't understand why this ?. is needed because the type doesn't include null | undefined. Do you have more context?
image

@romgrk romgrk added component: data grid This is the name of the generic UI component, not the React module! feature: Keyboard editing Related to the pickers keyboard edition labels Aug 18, 2023
@mui-bot
Copy link

mui-bot commented Aug 18, 2023 •
edited
Loading

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-10081--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms -200.8 -20.1 -200.8 -115.5 66.656
Sort 100k rows ms 665.8 1,400.7 1,380 1,149.96 265.134
Select 100k rows ms 598 810.8 713.3 711.38 68.567
Deselect 100k rows ms 126.2 308.3 186.2 200.2 59.444

Generated by 🚫 dangerJS against c4f1bfe

@romgrk romgrk marked this pull request as ready for review August 18, 2023 20:47
@MBilalShafi
Copy link
Member

MBilalShafi commented Aug 20, 2023 •
edited
Loading

I don't understand why this ?. is needed because the type doesn't include null | undefined. Do you have more context?

IIRC, currentPageRows appeared to be undefined for some reason or not held the current row index.

we need apiRef.current.getRowId to properly support the getRowId prop.

This sounds better, a single source of truth to get the rowId should reduce the chances of errors.

@romgrk
Copy link
Contributor Author

romgrk commented Aug 28, 2023

IIRC, currentPageRows appeared to be undefined for some reason or not held the current row index.

I've looked at the code, currentPageRows is generated by enrichPageRowsWithPinnedRows which return [...something], so it cannot be undefined, IIUC. I think maybe the ?. was meant to go after currentPageRows[rowIndex]? But then it means our typing for getRowIdFromIndex is incorrect, and I don't like that. But I'm not sure how to reproduce that issue.

@romgrk
Copy link
Contributor Author

romgrk commented Aug 29, 2023

I've fixed what needed to be fixed, I'll leave the ?. where it was intended to be for now.

@romgrk romgrk merged commit e743bf5 into mui:master Aug 31, 2023
@romgrk romgrk deleted the fix-keyboard-error-empty-rows branch August 31, 2023 01:35
oliviertassinari
oliviertassinari reviewed Aug 31, 2023
View reviewed changes
packages/grid/x-data-grid/src/tests/keyboard.DataGrid.test.tsx
@@ -715,4 +715,19 @@ describe('<DataGrid /> - Keyboard', () => {
fireEvent.keyDown(firstCell, { key: 'ArrowDown' });
expect(virtualScroller.scrollLeft).to.equal(0);
});

it('should not throw when moving into an empty grid', async () => {
Copy link
Member

@oliviertassinari oliviertassinari Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't look needed:

Suggested change
it('should not throw when moving into an empty grid', async () => {
it('should not throw when moving into an empty grid', () => {

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaviendelangle flaviendelangle flaviendelangle left review comments

@oliviertassinari oliviertassinari oliviertassinari left review comments

@cherniavskii cherniavskii cherniavskii approved these changes

@MBilalShafi MBilalShafi Awaiting requested review from MBilalShafi

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Keyboard editing Related to the pickers keyboard edition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[data grid] Pressing arrow down and page down on an empty grid throws exception.
6 participants
@romgrk @mui-bot @MBilalShafi @oliviertassinari @flaviendelangle @cherniavskii