[DataGrid] Fix rendering issues

[dtassone]

fix #1309
fix #1323

fix #481, done as rowIndex depends on sorted rows

[oliviertassinari]

Shouldn't it be a draft? I'm asking because, for instance, there are no descriptions in the PR, in the new API.

[oliviertassinari]

I don't know about the others, It's really hard for me to see what line fix what nor if each fix is tested/correctly documented (which is probably more important).
For instance, it says fix #1295 but it's still wrong, event propagates in the portal (using the reproduction on the issue, e.g. Backspace leads to an infinite loop).

Could we stop bunding bug fixes, especially if there are not directly related or don't need to be (e.g. #481)? How can we expect the team to review the changes?

I would highly encourage we close this PR and restart with smaller chunks, one issue at a time. Smaller iteration helps to be more methodical, and rigorous, which I think we lack. Of course, it takes more time, in the beginning. It would be setting the right example for others.
Or even consider the following, in one year from now, we have a regression on some behavior, being able to blame to see exactly what was done for a specific issue save so much time (I feel that this happens often in the main repo, e.g. today for me with a regression on the scroll in the docs, a 2 years old fix).

[dtassone]

#481 is fixed because I needed those methods to fix the issue that danail mentioned.

[dtassone]

I updated the description and remove #1295

[oliviertassinari]

fix #1300

This one seems to only be fixed on the surface. Could remove it from the list as well? I'm saying on the surface, because, for instance, if you press backspace, GRID_CELL_EXIT_EDIT is fired (why? the cell is not in edit mode), or the fact that if you press backspace, it re-renders the whole grid (slower), or the fact that it doesn't have a dedicate test case.

I think that without it, we would be almost ok. I still fail to see why we bundle a bug fix on virtualization (#1309) with a feature enhancement on sorting (#481). In my external context, I see no links between the two. But at least, #481 is only a small sidecar to the man focus #1309. I guess we don't need to update the documentation for it?

[DanailH]

In general, I support the idea of PR per ticket. I can also see that this fixes the one I mentioned yesterday in slack (https://material-ui.slack.com/archives/G011VC970AW/p1617094662001100).

Regarding #1300 - probably is better to have e separate effort for it (we can still have the enhancement here but if there is more work to be done that one shouldn't be closed).

Lastly - I know that the bugs this PR aims to fix are listed in the description but if the fix is not straightforward, simple change It would really help when reviewing if there are a couple of sentences explaining what the changes are.

[dtassone]

fix #1300

This one seems to only be fixed on the surface. Could remove it from the list as well? I'm saying on the surface, because, for instance, if you press backspace, GRID_CELL_EXIT_EDIT is fired (why? the cell is not in edit mode), or the fact that if you press backspace, it re-renders the whole grid (slower), or the fact that it doesn't have a dedicate test case.

I considered backspace to be an edit with a commit straightaway hence why it fires the GRID_CELL_EXIT_EDIT.
It rerenders the whole grid as we update the grid state. There is already a test case.
should allow to delete a cell directly if editable using delete key

I think that without it, we would be almost ok. I still fail to see why we bundle a bug fix on virtualization (#1309) with a feature enhancement on sorting (#481). In my external context, I see no links between the two. But at least, #481 is only a small sidecar to the man focus #1309. I guess we don't need to update the documentation for it?

The main issue of this PR was only raised on slack
Sorry for trying to sort out important issues in short delays.

[oliviertassinari]

I have added Fix #1319 in the pull request as it covers it. It also seems to have a dedicated test case 👍

[dtassone]

I have added Fix #1319 in the pull request as it covers it. It also seems to have a dedicated test case 👍

1319 is this PR

[oliviertassinari]

I considered backspace to be an edit with a commit straightaway hence why it fires the GRID_CELL_EXIT_EDIT.

The main part that is counter-intuitive to me is that the focus is on a nested input, not on the cell container. As far as I know, this interaction was designed for the case where we can to quickly empty a cell: "read mode > backspace > edit mode > empty > commit > exit event > read mode". It doesn't seem to make sense once the focus is move inside the cell, on a button, link, textbox.

1319 is this PR

My bad #1323, I got tricked by how GitHub replaces the URL when clicking a link :)

[oliviertassinari]

The main issue of this PR was only raised on slack. Sorry for trying to sort out important issues in short delays.

@dtassone I think that the intent is great: these GitHub issues deserved to be handled, soon or later 👍:100:. We have a bunch, which is fun to handle 😄.

I also think that the bundling is the opposite of the methodology @eps1lon and I have tried to generalize so far. I strongly don't think that we can grow the project, both in terms of product quality (we have a strong focus on this) and in terms of the size of the team by grouping GitHub issues that don't need to be. I think that it's really important and something we should rarely trade for something else (like convenience). Yes, it requires opening more pull requests, yes it forces us to think in more depth about each problem, which should translate into clearer thinking and stronger mastery (at the cost of spending more time and more concurrent effort) :).

[dtassone]

The main issue of this PR was only raised on slack. Sorry for trying to sort out important issues in short delays.

@dtassone I think that the intent is great: these GitHub issues deserved to be handled, soon or later 👍💯. We have a bunch, which is fun to handle 😄.

I also think that the bundling is the opposite of the methodology @eps1lon and I have tried to generalize so far. I strongly don't think that we can grow the project, both in terms of product quality (we have a strong focus on this in the core) and in terms of the size of the team by grouping GitHub issues that don't need to be. I think that it's really important and something we should rarely trade for something else (like convenience). Yes, it requires opening more pull requests, yes it forces us to think in more depth about each problem, which should translate into clearer thinking and stronger mastery (at the cost of spending more time and more concurrent effort) :).

While I agree to some extent. I believe that it can be counter-productive to not allow some flexibility to this policy in certain cases.

• Sometimes bugs are very closely related, you need the first fix to fix the second. Or your first fix impact the second. So you have to wait that the first bug to be merged to properly fix the second one. Should be small enough to group.
• There are regressions due to a lastly merged PR, different contributors work in the area, and need your fixes to approach their work properly. You're blocking them, issues get created, and it's breaking the component or crashing the page totally.

Something to consider here is that we are all contributing to the same component, so if I merge a bad PR then all contributors are impacted. In core, it's just the ppl that work on the same component. So issues are more critical, and more urgent in the grid, plus we have the commercial side IMHO.

[eps1lon]

Sometimes bugs are very closely related, you need the first fix to fix the second. Or your first fix impact the second. So you have to wait that the first bug to be merged to properly fix the second one. Should be small enough to group.

Note that you can always stack PRs on top of another.

1. create a branch fix-1 from main
2. create a branch fix-2 from fix-1
3. Open PR for fix-1 (no special attention)
4. Open PR for fix-2. Include a note that this PR is stacked on another one and add a link to the diff between fix-1...fix-2.

The idea is that we later want to isolate problems. While two bugs might seem closely related right now, they may no longer be closely related in the future. But since they were at some point, and you decided to fix both in a single commit, they become coupled indefinitely. Mapping past implementation to future implementation is no trivial task so when we do have the opportunity to make future readability easier at the expense of present work we should always pay that price. In the end we will always spend more time reading code than writing code.

There is some upfront cost in learning more advanced git techniques but I can guarantee you that being able to rebase confidently pays of big time in the long term.

[oliviertassinari]

@eps1lon 💯. I have updated https://www.notion.so/Engineering-Dojo-Kun-ec1615511f594782a8a2adeec3e25143 (The smaller a pull request is, the better).

The learnings I got from my past efforts on the project is that:

• As much as issues seem related on the surface (behavior) the root causes are in a large majority of the cases are unrelated. Based on all the bugs I have seen so far.
• The smaller the PR is, the less likely it will be challenged during a review, overlooked aspects will be found, the faster the change will be merged, and the faster we will unlock the rest of the team.
• Breaking down changes into smaller individual efforts very often yield higher quality solutions. Once made systematically, I believe it explains some of why Material-UI got so popular. Do less but do it really well. Which in a mature market where UI components are almost of commodity with high competition helps differentiate.
• So far, in most of the cases, the audience doesn't seem to care if a bug fix makes it in one release or the next one, a week later. It seems that the development cycles are a lot longer for them. Or maybe it's because if they need an urgent fix, they can change the version on their own instantly. It would be different if we were building a SaaS.

[oliviertassinari]

We got a regression linked to this PR: #1334.