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] Pass slot props to columnHeaders slot #12768

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

cherniavskii
Copy link
Member

Fixes #12749

@cherniavskii cherniavskii added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! customization: extend Logic customizability labels Apr 12, 2024
@mui-bot
Copy link

mui-bot commented Apr 12, 2024

Deploy preview: https://deploy-preview-12768--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 465f0b3

@@ -75,6 +75,7 @@ function GridHeaders() {
columnVisibility={columnVisibility}
columnGroupsHeaderStructure={columnGroupsHeaderStructure}
hasOtherElementInTabSequence={hasOtherElementInTabSequence}
{...rootProps.slotProps?.columnHeaders}
Copy link
Member Author

Choose a reason for hiding this comment

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

We tend to forget to pass slot props.
Is this a problem we want to solve in a general way?

A few ideas:

  1. Slot component:

    <Slot
      slotName="columnHeaders"
      ref={columnsContainerRef}
      visibleColumns={visibleColumns}
      // ...
    />
    
    function Slot<TSlotName extends keyof GridSlotProps & keyof GridSlotsComponent>({
      slotName,
      ...props
    }: GridSlotProps[TSlotName] & { slotName: TSlotName }) {
      const rootProps = useGridRootProps();
      const Component = rootProps.slots[slotName] as React.ElementType;
      return <Component {...props} {...rootProps.slotProps?.[slotName]} />;
    }
  2. useSlot hook:

    const [Slot, slotProps] = useSlot('columnHeaders');
    // + ESLint rule to enforce the usage of `slotProps`
    
    <Slot
      slotName="columnHeaders"
      ref={columnsContainerRef}
      visibleColumns={visibleColumns}
      {...slotProps}
    />
    
    const useSlot = <TSlot extends keyof GridSlotProps & keyof GridSlotsComponent>(slot: TSlot) => {
      const rootProps = useGridRootProps();
      const Component = rootProps.slots[slot];
      const slotProps = rootProps.slotProps?.[slot];
      return [Component, slotProps] as const;
    };

cc @mui/xgrid

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can make an eslint rule for 2, we can probably make one to just enfore it with <rootProps.slots.x {...rootProps.slotProps.x} />. I would prefer that. After that option 2. After that option 1.

Copy link
Member

@flaviendelangle flaviendelangle Apr 17, 2024

Choose a reason for hiding this comment

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

On the pickers and tree view we are always using useSlotProps (except on one instance of a transition component where it's not really doable).
That way we always have the same DX and we didn't forget to pass any slot props for a very long time.
This is a hook from the core team so it also improve the consistency of the codebase and allow to support callback version of the slotProps (which you don't from what I can see).

@cherniavskii cherniavskii marked this pull request as ready for review April 12, 2024 17:44
@trbagley-gresham
Copy link

Is it also worth exporting GridColumnHeadersProps as part of this PR so that GridColumnHeaders can easily be extended?

@cherniavskii
Copy link
Member Author

@trbagley-gresham You can use GridSlotProps['columnHeaders'] instead

Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

I assume the suggested improvement of the larger problem will be handled separately, right?

@cherniavskii cherniavskii enabled auto-merge (squash) April 19, 2024 18:16
@cherniavskii cherniavskii merged commit 48c34f8 into mui:master Apr 19, 2024
18 checks passed
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! customization: extend Logic customizability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Context menu in column headers
6 participants