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] Review on the filtering feature #633

Closed
26 of 30 tasks
oliviertassinari opened this issue Nov 22, 2020 · 11 comments
Closed
26 of 30 tasks

[DataGrid] Review on the filtering feature #633

oliviertassinari opened this issue Nov 22, 2020 · 11 comments
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module!

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 22, 2020

This is a follow-up after #411. I have tried to list all the improvement opportunities we can potentially leverage. This is meant to polish the feature so we can release it. By somewhat order of importance:

  • 0. Move multi-filtering support to XGrid
  • 1. Add documentation
  • 2. Add tests
  • 3. I think that the toolbar should be opt-in. This is too opinionated to be displayed by default. It's only something we should probably only include in XGrid, considering the pain around react-table is about the painful integration with the UI and that ag-Grid makes its toolbar paid.
  • 4. The menu should open over the column, taking into account the alignment:

position

position 2

  • 5. There is a double border on the toolbar

Capture d’écran 2020-11-22 à 22 10 26

  • 6. The menu jumps when closing, the content should stay identical. It feels cheap:

jump

  • 7. The toolbar actions should get a description. At least, at title in addition to the aria-label. We might want to inline the description inside it or allow developers to do such if they need to.
  • 8. The clear filter icon is not correctly aligned:

Capture d’écran 2020-11-22 à 22 24 18

  • 9. I would suggest we remove the clear button, individual arrows are likely enough:

Capture d’écran 2020-11-22 à 22 25 59

  • 10. The description of the add filter button is confusing. I think that it should be "Add filter", instead of "Filter".

Capture d’écran 2020-11-22 à 22 27 36

  • 11. The padding should be inside the scroll container

Capture d’écran 2020-11-22 à 22 30 34

  • 12. I think that we should remove the tabs. It won't scale and doesn't fit into the use cases. For instance with the column picker. It will be better to have a denser display vertically longer and horizontally shorter

Capture d’écran 2020-11-22 à 22 31 44

  • 13. The columns should be displayed in 1D, the mental model is one dimensional. I go confused, I miss interpreted the order I thought the list was collapsing vertically, but it collapses horizontally as it turns out

Capture d’écran 2020-11-22 à 22 33 24

  • 14. The column picker should have a search field.
  • 15. There are two scrollbars:

Capture d’écran 2020-11-22 à 22 35 44

  • 16. A switch would work better than a checkbox: https://next.material-ui.com/components/switches/#when-to-use for the column picker.
  • 17. A drag & drop for the column picker to reorder the columns would be 👌 but I think I should open a new issue for it. It's outside of the scope of the filters
  • 18. The popup should have rounded borders

Capture d’écran 2020-11-22 à 22 40 42

  • 19. The popup should leverage the existing components (change the elevation) not apply custom CSS, to replace:

Capture d’écran 2020-11-22 à 22 42 20

  • 20. We likely want to have a min-height for the popup, in the following configuration, the useful area is too small. We can only see one row, and the others are hidden:

Capture d’écran 2020-11-22 à 22 44 38

  • 21. .panelMainContainer or .panelFooter isn't a class name we can allow, it breaks the convention.
  • 22. We should try to replace padding: 10 with a value that fits the 4px spacing grid, for design consistency.
  • 23. This inline style duplicates with the CSS, we can drop it:

https://github.com/mui-org/material-ui-x/blob/b6e9a68edf32b3bcb143fa12f48df89bdec058bc/packages/grid/_modules_/grid/components/tools/FilterForm.tsx#L128

  • 24. All stylesheets should be named, assuming we want them to be public and allow customizations.

https://github.com/mui-org/material-ui-x/blob/b6e9a68edf32b3bcb143fa12f48df89bdec058bc/packages/grid/_modules_/grid/components/tools/FilterForm.tsx#L24

  • 25. The style of .panelFooter can be simplified, there are CSS properties that have no effect. I also think that 8px instead of 12px for the padding would work better, a higher density would probably be fine. Blueprint and Elastic UI are all-in.
  • 26. I feel that the icon menu draws too much attention when hovering the column header. I think that we could might it lighter and its current color only when hovered. In the following screenshot, I hover the arrow, not the icon button:

Capture d’écran 2020-11-22 à 23 01 43

  • 27. We miss a max-height for the popup. It becomes unusable once too large. This one is too high:

Capture d’écran 2020-11-22 à 23 04 58

Same problem with the column picker view in worse when the data table is tall (~1000px), I can't use it:

Capture d’écran 2020-11-22 à 23 08 30

  • 28. Assuming the toolbar is opt-in and only available in XGrid, I would encourage this API for the developers:
function Header() {
  return (
    <GridToolbar>
      <GridToolbarFilter />
      <GridToolbarColumn />
      <GridToolbarDensity />
    </GridToolbar>
  );
}

<XGrid component={{ Header }} />
  • 29. Maybe add negation operators (not equal, etc). Not sure, maybe it should only be available with a customization API.
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Nov 22, 2020
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 24, 2020

@dtassone
Copy link
Member

dtassone commented Nov 25, 2020

  • 31. Server-side filters as well

@dtassone
Copy link
Member

dtassone commented Nov 25, 2020

  • 32. Component customization. A user should be able to render a certain column menu or filters for a column.

@dtassone
Copy link
Member

  • 33. String filter input value can be an autocomplete field

@dtassone
Copy link
Member

dtassone commented Nov 25, 2020

  • 34. Add prop to preset the filter (same as sortModel)

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 25, 2020

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 27, 2020

  • 39. on hide column, menu anchor is lost 39

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 30, 2020

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 30, 2020

  • 41. I got confused a couple of times now. I think that clicking the filter button should toggle the popup, not only open it.

close

dtassone added a commit to dtassone/material-ui-x that referenced this issue Dec 1, 2020
@dtassone
Copy link
Member

dtassone commented Dec 1, 2020

  • 42. User should be able to select filter operators with a string ie column.filterOperators = {['contains', 'startWith']}

dtassone added a commit to dtassone/material-ui-x that referenced this issue Dec 1, 2020
dtassone added a commit to dtassone/material-ui-x that referenced this issue Dec 1, 2020
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 12, 2020

I have created a new issue for toolbar #726, tests & documentation are work in progress: #644, #715. For the other improvements, I think that it's best to ship the filters and get real feedback from developers, with the frequency, we can know what's more important, a lot better than guessing.

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

No branches or pull requests

2 participants