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.

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

Forward ref in data grid element #5202

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Forward ref in data grid element #5202

merged 1 commit into from
Aug 31, 2020

Conversation

jeiea
Copy link
Contributor

@jeiea jeiea commented Aug 29, 2020

Close #5194

DatagridRow ref doesn't cover expanded row. It can be controversial in my thought.

I made this commit on next branch because react guides use of forwardRef is breaking change.

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

👍

@fzaninotto
Copy link
Member

DatagridRow ref doesn't cover expanded row. It can be controversial in my thought.

Can you elaborate?

@jeiea
Copy link
Contributor Author

jeiea commented Aug 31, 2020

Currently DatagridRow is fragment, which cannot be assigned a ref.
I assigned ref to TableRow and didn't take account of the following div (expanded row).
For now I don't need ref of expanded row so it's fine to me, but I don't know how others think about this.
DatagridRow's ref doesn't include expanded row. I thought this fact cannot be clear.

@fzaninotto
Copy link
Member

Yet I don't see any other solution - a <tbody> cannot contain anything other than <tr>, so we cannot enclose the two table rows with anything other than a fragment.

We'll go with your change, and if the absence of ref on the expand row ever becomes a problem, we'll deal with it then.

@fzaninotto fzaninotto merged commit ede187b into marmelab:next Aug 31, 2020
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 3.9.0 milestone Aug 31, 2020
@jeiea jeiea deleted the next branch September 3, 2020 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants