Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Potential fix for grid row state propagation #15627
base: master
Are you sure you want to change the base?
[DataGrid] Potential fix for grid row state propagation #15627
Changes from 16 commits
45f52f9
7d6833c
e73f872
9b63559
28ed635
b3d87f7
a1db0cb
9295e5d
cbd3793
72507ac
d02033b
33cc4c9
76d22ae
1f95e27
f89f70e
28373b7
8c3d5cb
1b60107
d84764d
ce54c27
c61278c
e3eb6dd
c22c4d9
2810949
1417385
4e14aa7
b705c62
c5160ea
7ec0af7
fa8f8cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romgrk It should be safe to cherry-pick this PR to v7 once we merge it. Any objections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is moving this block required?
I prefer to keep functional components organized as if it were a class component:
In particular for large components, it helps readability a lot to keep all render code grouped rather than spread in-between "class methods".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like grouping logic, but here the balance is between a pretty large dead code path (defining an anyonymous function that should never be called, unless rowNode exists) vs. grouping logic. Not that clear what's more readable.
After this change, if you were to accidentally remove the rowNode check on component-level, the types would break (good imho), which is better than adding an extra conditional in the getCell that would open up the potential for using the compnent in currently unintended/undesigned ways.
Probably the right question here to ask is why the component needs to (try to) mount without a rowNode to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, the answer is often that the codebase went through many architectural & maintainer changes, and we haven't got around to refactoring everything.