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

[data grid] Add a default valueParser and valueFormatter to singleSelect columns #5615

Closed
Tracked by #3287
joserodolfofreitas opened this issue Jul 26, 2022 · 2 comments · Fixed by #7684
Closed
Tracked by #3287
Assignees
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! dx Related to developers' experience v6.x
Milestone

Comments

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Jul 26, 2022

Summary

Add a default valueParser valueFormatter to singleSelect columns

Motivation

DX: To avoid duplicate code in cell editing, filtering and quick filtering

@joserodolfofreitas joserodolfofreitas changed the title Add a default valueParser valueFormatter to singleSelect columns to avoid duplicate code in cell editing, filtering and quick filtering [data grid] Add a default valueParser and valueFormatter to singleSelect Jul 26, 2022
@joserodolfofreitas joserodolfofreitas added breaking change dx Related to developers' experience labels Jul 26, 2022
@joserodolfofreitas joserodolfofreitas changed the title [data grid] Add a default valueParser and valueFormatter to singleSelect [data grid] Add a default valueParser and valueFormatter to singleSelect columns Jul 26, 2022
@joserodolfofreitas joserodolfofreitas added component: data grid This is the name of the generic UI component, not the React module! v6.x labels Jul 26, 2022
@m4theushw
Copy link
Member

m4theushw commented Nov 22, 2022

The following line could be moved to a valueParser.

const formattedTargetValue = getValueFromValueOptions(target.value, valueOptionsFormatted);

@joserodolfofreitas joserodolfofreitas self-assigned this Nov 22, 2022
@m4theushw
Copy link
Member

The idea of this breaking change came from https://github.com/mui/mui-x/pull/4317/files#r843910779. The problem that justifies adding the value formatter is the following. First we added the singleSelect column type supporting valueOptions as a list of simple values, e.g.

{
  type: 'singleSelect',
  valueOptions: ['Brazil', 'France', 'Spain']
}

as well as a list of objects, e.g.

{
  type: 'singleSelect',
  valueOptions: [
    { value: 'br', label: 'Brazil' },
    { value: 'fr', label: 'France' },
    { value: 'es', label: 'Spain' },
  ]
}

When a list of object is provided as valueOptions, the label is used inside the menu but, once the dropdown is closed, the user sees value.

image

To make singleSelect usable, developers had to provide their own cell renderer or value formatter. Later, the quick filtering was implemented and it had a built-in logic for this column type where it checked not only value but also the label. Basically, it implemented a kind of value formatter in

if (iterableColumnValues) {
iterableColumnValues.forEach((option) => {
// for each valueOption, check if the formatted value
let optionValue;
let optionLabel;
if (typeof option === 'object') {
optionValue = option.value;
optionLabel = option.label;
} else {
optionValue = option;
if (valueFormatter) {
optionLabel = valueFormatter({ value: option, field, api: apiRef.current });
} else {
optionLabel = option;
}
}
if (optionLabel.slice(0, value.length).toLowerCase() === value.toLowerCase()) {
if (!potentialValues.includes(optionValue)) {
potentialValues.push(optionValue.toString());
}
}
});
}

This logic wouldn't be needed if we had added a default value formatter to singleSelect to convert br to Brazil by looking at the options provided. By doing so, the quick filter would only have to compare against the formatted value:

diff --git a/packages/grid/x-data-grid/src/colDef/gridSingleSelectOperators.ts b/packages/grid/x-data-grid/src/colDef/gridSingleSelectOperators.ts
index c550564ea..88f815a85 100644
--- a/packages/grid/x-data-grid/src/colDef/gridSingleSelectOperators.ts
+++ b/packages/grid/x-data-grid/src/colDef/gridSingleSelectOperators.ts
@@ -13,50 +13,14 @@ const parseObjectValue = (value: GridFilterItem) => {
   return value.value;
 };
 
-export const getGridSingleSelectQuickFilterFn = (
-  value: any,
-  column: GridColDef,
-  apiRef: React.MutableRefObject<GridApiCommunity>,
-) => {
-  if (!value) {
+export const getGridSingleSelectQuickFilterFn = (quickFilterValue: any) => {
+  if (!quickFilterValue) {
     return null;
   }
-  const { valueOptions, valueFormatter, field } = column;
 
-  const potentialValues = [parseObjectValue(value).toString()];
-
-  const iterableColumnValues =
-    typeof valueOptions === 'function' ? valueOptions({ field }) : valueOptions || [];
-
-  if (iterableColumnValues) {
-    iterableColumnValues.forEach((option) => {
-      // for each valueOption, check if the formatted value
-      let optionValue;
-      let optionLabel;
-      if (typeof option === 'object') {
-        optionValue = option.value;
-        optionLabel = option.label;
-      } else {
-        optionValue = option;
-        if (valueFormatter) {
-          optionLabel = valueFormatter({ value: option, field, api: apiRef.current });
-        } else {
-          optionLabel = option;
-        }
-      }
-
-      if (optionLabel.slice(0, value.length).toLowerCase() === value.toLowerCase()) {
-        if (!potentialValues.includes(optionValue)) {
-          potentialValues.push(optionValue.toString());
-        }
-      }
-    });
-  }
-
-  return ({ value: columnValue }: GridCellParams): boolean => {
-    return columnValue != null
-      ? potentialValues.includes(parseObjectValue(columnValue).toString())
-      : false;
+  return ({ value, formattedValue }: GridCellParams): boolean => {
+    const valueToTest = formattedValue == null ? value : formattedValue;
+    return valueToTest.toLowerCase() === quickFilterValue.toLowerCase();
   };
 };

At that time, adding a default value formatter was not allowed because it would be a breaking change. Maybe some user preferred to select Brazil but see br.

About the value parser, I think it was a typo from @alexfauquette because he mentions "value parser" in https://github.com/mui/mui-x/pull/4317/files#r843616217 but later he gives an example with value formatter. However, we do can also add a value parser because the logic from #5615 (comment) is the same in

value = getValueFromValueOptions(value, currentValueOptions);
and valueParser was designed to be called both for edit components and filter components. The value parser to be added only needs to act when the native select is used. Its job is to receive the value selected by the user (always as a string) and return the value in valueOptions with the correct type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! dx Related to developers' experience v6.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants