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] Default toolbar setup #574

Merged
merged 18 commits into from
Nov 13, 2020

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Nov 12, 2020

This pull request aims to add an empty toolbar that will sit at the top of the data grid (above the columns and the header component it such is provided). It is structured in a way that will allow us to be easily extracted in the future if we decided to do so. To add buttons (features) in the Toolbar on needs to create a separate component for that feature (eg. ColumnPicker) and implement the functionality there. After it is done the following structure should be followed when adding the feature to the toolbar:

<DefaultToolbar>
  <ColumnPicker />
  ...
</DefaultToolbar>

Questions that need to be answered in this Draft PR:

  • What to do if there are no buttons/features added (like it is now). The toolbar is visible although it is empty. I think it would be better to hide it if no children are provided.
  • Once we agree that it is a viable solution I will add the needed tests and documentation, for now, there is none.
  • The styling of the Toolbar - for now I just added a fixed min-height, centered the items vertically, added a border, and a background. The min-height matches the one we have for an MUI button with size small (a similar approach was taken for the footer where the min-height matches the height of the TablePagination component).

Closes #516

@DanailH DanailH added priority: important This change can make a difference new feature New feature or request labels Nov 12, 2020
@DanailH DanailH self-assigned this Nov 12, 2020
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Looks like a good skeleton. Note that we need to fix #502 before we can display anything in the header/toolbar. I believe the solution is to reorganize the grid to have the AutoSizer nested

@DanailH
Copy link
Member Author

DanailH commented Nov 12, 2020

Nice, ok let's see what @dtassone will say about the skeleton because I need this merged in order to start with either the column picker or the density selector. In the meantime, I can take a look at the AutoSizer issue.

@DanailH
Copy link
Member Author

DanailH commented Nov 13, 2020

If this will be the way to go should I proceed with adding the documentation, hiding the toolbar if it is empty, and convert it to a normal PR?

@DanailH DanailH marked this pull request as ready for review November 13, 2020 12:35
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Why would somebody use the toolbar over the header?

DanailH and others added 3 commits November 13, 2020 16:24
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
…erial-ui-x into feature/DataGrid-516-toolbar
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 13, 2020

I have updated the pull-request to go into the header direction:

<DataGrid
  components={{
    header: () => (
      <GridToolbar>
        <GridToolbarFilter />
        <GridToolbarDensity />
      </GridToolbar>
    )
  }}
/>

@DanailH
Copy link
Member Author

DanailH commented Nov 13, 2020

I have updated the pull-request to go into the header direction:

<DataGrid
  components={{
    header: () => (
      <GridToolbar>
        <GridToolbarFilter />
        <GridToolbarDensity />
      </GridToolbar>
    )
  }}
/>

Ok so probably I misunderstood your previous comment. You want basically for the toolbar to be the header in terms of the API we expose.

DanailH and others added 4 commits November 13, 2020 19:20
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
DanailH and others added 5 commits November 13, 2020 19:21
….tsx

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
….stories.tsx

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
…d.js

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@DanailH
Copy link
Member Author

DanailH commented Nov 13, 2020

Ok, I've cleaned it up further, so now to overwrite the GridToolbar one needs to provide a header component.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Great, I had a sync with @dtassone, and we are all aligned for pushing further with this API :)

@DanailH
Copy link
Member Author

DanailH commented Nov 13, 2020

Great, I had a sync with @dtassone, and we are all aligned for pushing further with this API :)

Nice, that is still easily abstractable tho, so if we decide to go with the Provider composition it will still be fairly easy to do it.

@DanailH DanailH merged commit d98964f into mui:master Nov 13, 2020
- `hideToolbar`: If `true`, the toolbar component is hidden.

You can replace the default toolbar by providing your own implementation.
You can replace the default toolbar by providing a `header` component.
Copy link
Member

Choose a reason for hiding this comment

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

I'm lost with this. I thought we would add a toolbar slot

Copy link
Member

Choose a reason for hiding this comment

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

This PR is 3 months old

@@ -39,14 +39,6 @@ export const useComponents = (
[componentOverrides, componentProps],
);

const toolbarComponent = React.useMemo(
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request priority: important This change can make a difference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Add toolbar
3 participants