[DataGrid] Remove focus on cell when its row is removed from the data

[flaviendelangle]

Fixes #1958

[flaviendelangle]

The issue occurred when removing a row that had a cell focused when removing it.
The next time we clicked on the document, it published the event GRID_CELL_FOCUS_OUT which tried to pass the value of the cell but unsucessfully.

I'm not sure what the best solution here is.
I went for a simple useEffect that checks if the focused cell has been removed whenever the rows prop changes, which solves the issue.

Of course this would require a new test once a solution is decided.

Another issue here is that the typing of getCellValue assures that is does return a value. But if we pass an invalid id, it returns undefined. I think that in a development environment it should throw an explicit error instead of letting the rest of the code crash with errors like cannot access X of undefined.

getRow: (id: GridRowId) => GridRowModel;

[dtassone]

The issue occurred when removing a row that had a cell focused when removing it.
The next time we clicked on the document, it published the event GRID_CELL_FOCUS_OUT which tried to pass the value of the cell but unsucessfully.

I'm not sure what the best solution here is.
I went for a simple useEffect that checks if the focused cell has been removed whenever the rows prop changes, which solves the issue.

Of course this would require a new test once a solution is decided.

Another issue here is that the typing of getCellValue assures that is does return a value. But if we pass an invalid id, it returns undefined. I think that in a development environment it should throw an explicit error instead of letting the rest of the code crash with errors like cannot access X of undefined.

getRow: (id: GridRowId) => GridRowModel;

The solution looks fine.
I don't think we should throw an error if the id is invalid, I think it should just return null and we should update the type.
One could check if a row is in in the grid using apiRef and getRow

[flaviendelangle]

OK, I'll add some test for the current behavior change
And for the type change I'll do another PR after

[oliviertassinari]

The bug is fixed: https://codesandbox.io/s/zen-darkness-fyjex 👍. I wonder about:

1. Would we have the same issue with a removed column? Maybe we shouldn't are yet.
2. Is the cleanup done soon enough? It seems to, it runs in the same effect timing than convertGridRowsPropToState.

[oliviertassinari]

Suggested change

	it('should reset focus when removing the row containing the focus cell', async () => {
	it('should reset focus when removing the row containing the focus cell', () => {

[flaviendelangle]

1. I think that there is a broader subject than just row removal here indeed. Could there be the same behavior for other things than just focus ?

2. I also think so.

[oliviertassinari]

Suggested change

	it('should not reset focus when removing a row not containing the focus cell', async () => {
	it('should not reset focus when removing a row not containing the focus cell', () => {

[magrinj]

@flaviendelangle Hey ! Thanks for the fix, when this one will be published in an npm release ?

[oliviertassinari]

@magrinj Next up on our release round-robin is Damien. The last release was 2 weeks ago, so I assume it will happen soon or later.