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] Implement multipleSelect column type #11325

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

martinjlowm
Copy link

@martinjlowm martinjlowm commented Dec 6, 2023

This adds a data grid column type for selecting multiple items.

This is based on Jim's work at #8099, see that PR for further details.

Closes

@mui-bot
Copy link

mui-bot commented Dec 6, 2023

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running yarn l10n
  • Clean files with yarn prettier.

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

Updated pages:

Generated by 🚫 dangerJS against a5811ad

@oliviertassinari oliviertassinari added CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 component: data grid This is the name of the generic UI component, not the React module! labels Dec 6, 2023
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@romgrk
Copy link
Contributor

romgrk commented Jan 22, 2024

@martinjlowm Sorry about the delay and the merge conflicts, we're in the process of releasing v7 so the breaking changes have been taking priority as we only release a major version once a year. If you can solve the conflicts for the current head commit, I'll review the PR.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2024
@martinjlowm
Copy link
Author

@romgrk there you go 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this file has this diff, maybe something happened to it on the default branch.

Copy link
Author

Choose a reason for hiding this comment

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

The commit comes from a Pretter run - just took a look at the diff off GitHub and there it looks like there are two additional spaces in the first column prior to this.

Copy link
Author

@martinjlowm martinjlowm Jan 23, 2024

Choose a reason for hiding this comment

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

Never mind - thought your comment was pinned to the column definition file. It's renamed to index.md now.

Comment on lines +11 to +12
valueFormatter(params) {
const { id, field, value, api } = params;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will conflict with #11573, need to watch out for this one.

Comment on lines 15 to 17
if (!isMultipleSelectColDef(colDef)) {
return [];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check necessary if the formatter is for this column?

Copy link
Contributor

Choose a reason for hiding this comment

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

This formatter returns [] in a few places, isn't it supposed to return a string?

Copy link
Author

Choose a reason for hiding this comment

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

Good question - it's a continuation of Jim's work. I'd agree that streamlining the return type here makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

https://mui.com/material-ui/api/autocomplete/#autocomplete-prop-multiple - we explicitly set multiple on the autocomplete component, but that I would assume would mean that the succeeding string branches should be arrays as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

But IIUC, valueFormatter is only for formatting to string. One of its use-cases is filtering (full-text search). I'm not sure how else it's used. Maybe someone else can add more context.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://next.mui.com/x/react-data-grid/column-definition/#value-formatter says:

Note, that the value returned by valueFormatter is only used for rendering purposes. Filtering and sorting are based on the raw value (row[field]) or the value returned by valueGetter.

But this is wrong, the quickfilter code uses the formatted values for its filtering.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't wrap my head about the double Multiple in the file name, would you mind explaining what it refers to? Having more than one multiple-select?

Copy link
Author

Choose a reason for hiding this comment

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

I think the idea was that the multipleSelect type was used inside an Autocomplete component with the multiple flag set. So an indication of nesting.

Copy link
Author

@martinjlowm martinjlowm Jan 23, 2024

Choose a reason for hiding this comment

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

It doesn't look too good and there's a logical error - at least the autocomplete part doesn't assist with finding possible matches, which was what I would expect. Never mind this is actually the single select. Regardless, it'd be nice with an example of the multiple select in there.

Bildschirmfoto 2024-01-23 um 21 25 33

@romgrk romgrk requested a review from cherniavskii January 23, 2024 20:12
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@MBilalShafi MBilalShafi changed the base branch from next to master March 21, 2024 02:31
@scottjrainey
Copy link

What would I need to do to address this PR being out of date? I'm a Pro customer and this would be very useful for our use case. I would be happy to update the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 component: data grid This is the name of the generic UI component, not the React module! feature: Editing Related to the data grid Editing feature new feature New feature or request PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants