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

[DataGrid] Fix components casing, rm context, refact GridComponent #707

Merged
merged 4 commits into from
Dec 8, 2020

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Dec 8, 2020

No description provided.

}),
[handleMouseDown],
);
useApiMethod<ColumnResizeApi>(apiRef, { startResizeOnMouseDown: handleMouseDown }, 'columnResizeApi')
Copy link
Member

@oliviertassinari oliviertassinari Dec 8, 2020

Choose a reason for hiding this comment

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

Shouldn't we do the opposite, have a generic props spreader we tap into? From what I understand, we are now leaking the implementation details of the column resizing feature to the view (if we want to make it composable, the view shouldn't know anything about this column resize feature).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this is the current way of building the grid and linking hooks to components.
In the previous implementation, all children from root had to forward a prop that they didn't need to be aware of to their child, which is property drilling and impact on component portability.
This way if you replace the full column header you can still use the resize functionality through the apiRef, and none of the components need to be aware of the separator props.

We might refactor that when do more refactoring on component customization but ATM, it's nice to have everything working in a homogeneous way.

Copy link
Member

@oliviertassinari oliviertassinari Dec 8, 2020

Choose a reason for hiding this comment

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

Sure for the current way of doing things. I was more interested in the long term direction. I was under the assumption that a generic props drilling would allow portability, make customizations easier. Isn't it how downshift and react-table behave? They have wildcard props you need to spread on specific components, input, pop-up, wrapper, header cell, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Props drilling doesn't bring portability as if you change the component structure you have to change the props, and maybe add more drilling and so on... Also, it doesn't scale, imagine you have 20 props to pass around, then need to refactor, hell!
For downshift, it seems that they pass a bunch of functions to a children function that the react tree can use. However if a root component contains another component within it, it might need some props that are unrelated to itself. So it would have 2 options either forward the prop or use the context. The latter is better, as it doesn't need to be aware of its child component props, it's more portable and it's more scalable. So basically that's what we do through the apiRef.
We can imagine a more flexible approach to customization but that would be another PR and discussion.

@dtassone dtassone merged commit ac59793 into mui:master Dec 8, 2020
@dtassone dtassone changed the title [DataGrid] Some small refactoring [DataGrid] Fix components casing, rm context, refact GridComponent Dec 8, 2020
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants