-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Add default value formatter to singleSelect
#7290
Conversation
These are the results for the performance tests:
|
cf05b26
to
89a84ea
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| React.JSXElementConstructor<GridFilterInputValueProps> | ||
| React.JSXElementConstructor<GridFilterInputMultipleValueProps> | ||
| React.JSXElementConstructor<GridFilterInputMultipleSingleSelectProps>; | ||
InputComponent?: React.JSXElementConstructor<any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is to solve a dependency cycle. Actually it should be already any
because we allow to customize the props too.
@@ -10,7 +10,7 @@ export const getGridStringQuickFilterFn = (value: any) => { | |||
return null; | |||
} | |||
const filterRegex = new RegExp(escapeRegExp(value), 'i'); | |||
return ({ value: columnValue }: GridCellParams): boolean => { | |||
return ({ formattedValue: columnValue }: GridCellParams): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a bug in the quick filter. It doesn't use the formatted value to check if the row satisfies the search string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the single select, you're modifying the default behavior. It should be reflected in the docs
https://next.mui.com/x/react-data-grid/filtering/#custom-filtering-logic
packages/grid/x-data-grid/src/colDef/gridSingleSelectColDef.tsx
Outdated
Show resolved
Hide resolved
import { getGridSingleSelectOperators } from './gridSingleSelectOperators'; | ||
import { getLabelFromValueOption } from '../components/panel/filterPanel/filterPanelUtils'; | ||
|
||
const isArrayOfObjects = (options: any): options is Array<{ value: any; label: string }> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the constraint of { value: any; label: string }
for option values is very limiting.
Currently, it's not possible to pass object structures different than the one mentioned. Any other structure should be transformed to fit { value: any; label: string }
shape before passing valueOptions
to the column.
I think we can improve the DX by allowing it to pass any object shape and extend the concept of getOptionLabel
further:
{
type: 'singleSelect',
// If the value option is an object and doesn't match the `{ value, label }` shape - throw an
// error or log error message suggesting adding `parseValueOption`
valueOptions: [
{ code: 'br', name: 'Brazil' },
{ code: 'it', name: 'Italy' }
],
parseValueOption: ({ code, name }) => ({ value: code, label: name }),
}
Then we can operate on the { value, label }
shape internally while allowing any custom object shapes.
It's pretty similar to getRowId
.
I know it would increase the scope of the PR, but we can take the opportunity to make breaking changes.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to #7290 (comment) and #7290 (comment)
Initially I implemented getOptionLabel
only to be an escape hatch if someone has added a custom value formatter to format the options, but now it makes sense to move getOptionLabel
to the column definition object and pass it automatically to the components. In that way, the job of parseValueOption
is split between two new attributes: getOptionValue
and getOptionLabel
.
I know it would increase the scope of the PR, but we can take the opportunity to make breaking changes.
I would propose to do this in a follow-up if we want to do the right thing (add tests and update docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose to do this in a follow-up if we want to do the right thing
Agree! But should we put this PR on hold then? Otherwise, we will first add getOptionLabel
and then remove it in a few weeks. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can prefix getOptionLabel
with unstable_
to make it easier to remove in the follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m4theushw Should we proceed with getOptionLabel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot to answer this one. I don't think we need to prefix getOptionLabel
with unstable_
nor remove it in the follow-up PR. We can still support the prop in the edit components and also in the column definition. The most specific wins. Someone may want to provide a getOptionLabel
to the column definition and also customize the label only when editing the cell. In that case, developers will need to extend the edit component and pass a custom callback to the prop. What do you think?
The changes need to be done to support getOptionLabel
in the column definition are more or less the following:
diff --git a/packages/grid/x-data-grid/src/colDef/gridSingleSelectColDef.tsx b/packages/grid/x-data-grid/src/colDef/gridSingleSelectColDef.tsx
index 7b1863f8c..8b9c13f8e 100644
--- a/packages/grid/x-data-grid/src/colDef/gridSingleSelectColDef.tsx
+++ b/packages/grid/x-data-grid/src/colDef/gridSingleSelectColDef.tsx
@@ -11,6 +11,7 @@ const isArrayOfObjects = (options: any): options is Array<{ value: any; label: s
export const GRID_SINGLE_SELECT_COL_DEF: GridColTypeDef = {
...GRID_STRING_COL_DEF,
type: 'singleSelect',
+ getOptionLabel: getLabelFromValueOption,
valueFormatter(params) {
const { id, field, value, api } = params;
const colDef = params.api.getColumn(field);
diff --git a/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx b/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx
index eb1789550..5ba80dc7b 100644
--- a/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx
+++ b/packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx
@@ -61,10 +61,12 @@ function GridEditSingleSelectCell(props: GridEditSingleSelectCellProps) {
error,
onValueChange,
initialOpen = rootProps.editMode === GridEditModes.Cell,
- getOptionLabel = getLabelFromValueOption,
+ getOptionLabel: getOptionLabelProp,
...other
} = props;
+ const getOptionLabel = getOptionLabelProp || colDef.getOptionLabel;
+
const apiRef = useGridApiContext();
const ref = React.useRef<any>();
const inputRef = React.useRef<any>();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, instead of adding GridColDef['parseOptionValue']
I proposed above, in the follow-up PRs we will add:
GridColDef['getOptionLabel']
GridColDef['getOptionValue']
Is this correct?
If so, then it sounds good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly! I just don't want to do it here because I need to also update docs and add tests.
* @param {ValueOptions} value The current value option. | ||
* @returns {string} The text to be displayed. | ||
*/ | ||
getOptionLabel?: (value: ValueOptions) => React.ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be declared in gridColDef.ts
?
Also, should it return a string?
getOptionLabel?: (value: ValueOptions) => React.ReactNode; | |
getOptionLabel?: (value: ValueOptions) => string; |
@@ -69,6 +61,7 @@ function GridEditSingleSelectCell(props: GridEditSingleSelectCellProps) { | |||
error, | |||
onValueChange, | |||
initialOpen = rootProps.editMode === GridEditModes.Cell, | |||
getOptionLabel = getLabelFromValueOption, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't default getOptionLabel
be defined in gridSingleSelectColDef.tsx
?
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@@ -36,7 +36,6 @@ | |||
{ "name": "getGridNumericOperators", "kind": "Variable" }, | |||
{ "name": "getGridNumericQuickFilterFn", "kind": "Variable" }, | |||
{ "name": "getGridSingleSelectOperators", "kind": "Variable" }, | |||
{ "name": "getGridSingleSelectQuickFilterFn", "kind": "Variable" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include this one in the migration guide as well?
Part of #5615
The main purpose of this change is to allow that when users pass options as
[{ value: 'br', label: 'Brazil' }]
they will seeBrazil
after selecting a value. Currently, they seebr
. ThesingleSelect
column also used to apply the value formatter to the options, but this is now gone to be complaint with the purpose of the value formatter: a function to format the value displayed to the user when the cell is in view mode. As consequence, this will force users to pass an array of objects. To make the migration smoother, or even to allow to use a simple array, I added thegetOptionLabel
prop to the edit components. This props allows to customize the text is used for each option.I'll add the default value parser in a follow up.
Changelog
Breaking changes
singleSelect
column type now has a default value formatter that returns thelabel
correspoding to the selected value whenvalueOptions
is an array of objects. As consequence, any existing value formatter will not be applied to the individual options anymore, but only to the text of the cell. It is recommended to migratevalueOptions
to an array of objects to be able to add a custom label for each value. To override the label used for each option when the cell is in edit mode or in the filter panel, the following components now support agetOptionLabel
prop. This prop accepts a callback that is called with the item fromvalueOptions
and must return the new label.GridEditSingleSelectCell
GridFilterInputSingleSelect
GridFilterInputMultipleSingleSelect
getGridSingleSelectQuickFilterFn
function was removed. You can copy the old function and pass it to thegetApplyQuickFilterFn
property of thesingleSelect
column definition.