-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] New Toolbar component #14611
Conversation
Deploy preview: https://deploy-preview-14611--material-ui-x.netlify.app/ Updated pages: |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
<ColumnsPanelTrigger render={<ToolbarButton />}> | ||
<ViewColumnIcon fontSize="small" /> | ||
</ColumnsPanelTrigger> |
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.
Are these 2 snippets equal?
<ColumnsPanelTrigger render={<ToolbarButton />}>
<ViewColumnIcon fontSize="small" />
</ColumnsPanelTrigger>
<ColumnsPanelTrigger render={<ToolbarButton><ViewColumnIcon fontSize="small" /></ToolbarButton>} />
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.
They are, it comes down to preference. Do you think it's worth documenting this on the usage page?
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.
Yes, I think it's worth clarifying that children are automatically passed to the render
element. Otherwise, it might be a bit "magical" for an average user 😅
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.
Mentioned it here - let me know how that sounds.
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.
If I understood correctly, it makes more sense to use render prop if there's a need to access props or state.
<ColumnsPanelTrigger render={(props) => <ToolbarButton {...props} />}>
<ViewColumnIcon fontSize="small" />
</ColumnsPanelTrigger>
Is it possible to skip the render prop for simple use cases?
<ColumnsPanelTrigger>
<ToolbarButton>
<ViewColumnIcon fontSize="small" />
</ToolbarButton>
</ColumnsPanelTrigger>
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.
If I understood correctly, it makes more sense to use render prop if there's a need to access props or state.
That's a reason to pass a function to the render prop. The other case is to merge functionality and change the element rendered by a component.
Is it possible to skip the render prop for simple use cases?
In this example, no. The <ColumnsPanelTrigger />
and <ToolbarButton />
both render a <button />
, so you would end up with this:
<button>
<button>
<svg />
</button>
</button>
By using the render prop like: <ColumnsPanelTrigger render={<ToolbarButton />}><ViewColumnIcon /></ColumnsPanelTrigger>
, the styled button element rendered by <ToolbarButton />
is the only element that gets rendered but with the merged attributes from both elements:
<button>
<svg />
</button>
I will update the usage doc to try to explain this better.
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've made some updates to the Usage doc. Let me know if there's anything that could be clarified further.
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.
Thanks for the clarification, I understand it's one DX vs the other.
The other case is to merge functionality and change the element rendered by a component.
Would doing something like the following here avoid creating the nested button elements while supporting the third case below: 🤔
const { children, ...rest } = props;
if (React.isValidElement(children)) {
return React.cloneElement(children, rest);
}
My point was to skip the default <button />
element if there's no render prop and children are passed. In that case <ColumnsPanelTrigger />
would serve as a logic layer and the view layer would be delegated to the user.
So these would be the possible options for the users rendering the same output.
<ColumnsPanelTrigger render={<ToolbarButton />}>
<ViewColumnIcon fontSize="small" />
</ColumnsPanelTrigger>
<ColumnsPanelTrigger render={<ToolbarButton><ViewColumnIcon fontSize="small" /></ToolbarButton>} />
<ColumnsPanelTrigger>
<ToolbarButton>
<ViewColumnIcon fontSize="small" />
</ToolbarButton>
</ColumnsPanelTrigger>
I'm fine if we want to stick to the render prop only, it's just that "only with children" use-case could be really nice DX wise and pretty self-explanatory.
Consider this thread unblocking for the PR merge.
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.
My point was to skip the default element if there's no render prop and children are passed
I agree it might be a nice DX and less verbose, however, with these components we are also trying to ensure that the rendered HTML is using the correct semantics and aria attributes. Users may want to use the <ColumnsPanelTrigger />
outside of the <Toolbar />
and drop the <ToolbarButton />
:
<ColumnsPanelTrigger>
<ViewColumnIcon fontSize="small" />
</ColumnsPanelTrigger>
If we were to use the third option you suggested, it would result in invalid HTML; an <svg />
gets rendered with a bunch of attributes intended for a button.
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.
Fair enough, let's not risk semantic inconsistencies.
packages/x-data-grid/src/components/quickFilter/QuickFilter.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrew Cherniavskii <[email protected]> Signed-off-by: Kenan Yusuf <[email protected]>
Besides the latest open points LGTM |
<ColumnsPanelTrigger render={<ToolbarButton />}> | ||
<ViewColumnIcon fontSize="small" /> | ||
</ColumnsPanelTrigger> |
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.
If I understood correctly, it makes more sense to use render prop if there's a need to access props or state.
<ColumnsPanelTrigger render={(props) => <ToolbarButton {...props} />}>
<ViewColumnIcon fontSize="small" />
</ColumnsPanelTrigger>
Is it possible to skip the render prop for simple use cases?
<ColumnsPanelTrigger>
<ToolbarButton>
<ViewColumnIcon fontSize="small" />
</ToolbarButton>
</ColumnsPanelTrigger>
Adds a redesigned Toolbar component through a new composition API, documented here:
There are several related components that users can use to build a custom toolbar:
Note
This PR only adds the building blocks to create custom toolbars, and documentation for those components. I'm hoping we can release these in v7 for users to try out. There will be a follow up to update the default grid toolbar for v8 to use the new components. #15823
Closes #11584
Follow-up tasks:
QuickFilter
components inGridToolbarQuickFilter