[DataGrid] Enable using non-native Select in filter panel #4361

kyeongsoosoo · 2022-04-04T16:02:10Z

Preview: https://codesandbox.io/s/columntypesgrid-material-demo-forked-ll8bmp?file=/demo.tsx

This PR resolves the issue that Select component in Filter Panel only uses native UI.

Following the suggestion, I made 3 changes.

ClickAwayListener in Filter Panel uses onMouseUp mouseEvent.

<ClickAwayListener mouseEvent="onMouseUp" onClickAway={handleClickAway}>

When rootProps.componentsProps?.baseSelect.native is set to false, baseSelect component uses MUI component and MenuItem component is used instead of option tag.
rootProps.componentsProps?.baseSelect is passed to InputComponent when Select Component is used.

      SelectProps={{
        native: isSelectNative,
        ...rootProps.componentsProps?.baseSelect,
      }}

NOTE. To avoid breaking change, I set default native to true.

Before

2022-04-05.12.59.42.mov

After

2022-04-05.12.25.22.mov

mui-bot · 2022-04-04T16:13:09Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	230.9	435.5	404.9	346.54	94.194
Sort 100k rows	ms	449.8	964.8	711.5	691.28	168.064
Select 100k rows	ms	128.4	289.5	134.8	175.36	62.296
Deselect 100k rows	ms	96.6	298.9	250.1	190.82	75.948

Generated by 🚫 dangerJS against 3be4afd

alexfauquette

Supporting both native and non-native select seems to be a pain due to the switch between <option /> and <MenuItem />

What would you think about adding a helper component to switch between them more easily

const SelectOption = ({ isNative, ...props }) => isNative ? <option {...props} /> : <MenuItem {...props} />

packages/grid/x-data-grid/src/components/panel/filterPanel/GridFilterForm.tsx

alexfauquette · 2022-04-05T08:53:37Z

packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx

@@ -137,6 +137,7 @@ function GridEditSingleSelectCell(props: GridRenderEditCellParams & Omit<SelectP
      fullWidth
      {...other}
      {...rootProps.componentsProps?.baseSelect}
+      native={false}


That's the default value. I do not think users prop should be overridden

A solution could be to send a warning if other.native || rootProps.componentsProps?.baseSelect?.native explaining that EditSingleSelectCell does not support native select

Here is an example of a warning message:

mui-x/packages/grid/x-data-grid/src/hooks/features/columns/gridColumnsUtils.ts

Lines 295 to 305 in 3d2ff09

if (process.env.NODE_ENV !== 'production') {

if (!columnTypeWarnedOnce && !columnTypes[type]) {

console.warn(

[

`MUI: The column type "${type}" you are using is not supported.`,

`Column type "string" is being used instead.`,

].join('\n'),

);

columnTypeWarnedOnce = true;

}

}

Another solution is to support native select by adding an argument isNative to renderSingleSelectOptions such that it can switch between <MenuItem/> and <option />

Another solution is to support native select by adding an argument isNative to renderSingleSelectOptions such that it can switch between and

+1 for this option. It will be odd to have all selects working with native=false and only this one that doesn't.

I chose the latter solution(support native select).
I tested the change using Storybook, and I found that NativeSelect casts the value to a string.
To fix that issue, I copied the util getValueFromValueOptions from filterPanelUtils and formatted the value.

const handleChange = async (event: React.KeyboardEvent<HTMLInputElement>) => { setOpen(false); const target = event.target as HTMLInputElement; // NativeSelect casts the value to a string. const formattedTargetValue = getValueFromValueOptions(target.value, valueOptionsFormatted); const isValid = await api.setEditCellValue({ id, field, value: formattedTargetValue }, event); ...

packages/grid/x-data-grid/src/components/panel/filterPanel/GridFilterInputSingleSelect.tsx

m4theushw

Good job! I only think we could go one step further and reduce the amount of code duplicated. Sometimes only one or two props change.

m4theushw · 2022-04-05T13:25:32Z

packages/grid/x-data-grid/src/components/panel/filterPanel/GridFilterForm.tsx

-          ))}
+          {linkOperators.map((linkOperator) =>
+            isBaseSelectNative ? (
+              <option key={linkOperator.toString()} value={linkOperator.toString()}>


The props are the same for MenuItem and option. You could store in an OptionComponent variable which option component to use, then do:

const OptionComponent = ... linkOperators.map((linkOperator) => ( <OptionComponent key={...} value={...} /> ))

Same for the other components. This reduce code duplication.

packages/grid/x-data-grid/src/components/panel/filterPanel/GridFilterInputBoolean.tsx

kyeongsoosoo · 2022-04-05T17:32:28Z

@alexfauquette @m4theushw
Thanks for the review. I refactored some code following the suggestions.

alexfauquette

Changes look nice. Maybe putting the SelectOption in a dedicated file to avoid duplicated code, and for me it will be good.

packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx

alexfauquette · 2022-04-06T10:31:32Z

packages/grid/x-data-grid/src/components/panel/filterPanel/GridFilterForm.tsx

+interface SelectOptionProps {
+  isNative: boolean;
+  children: React.ReactNode;
+  value: string;
+}
+
+const SelectOption = ({ isNative, children, value, ...props }: SelectOptionProps) =>
+  isNative ? (
+    <option value={value} {...props}>
+      {children}
+    </option>
+  ) : (
+    <MenuItem value={value} {...props}>
+      {children}
+    </MenuItem>
+  );


Since this code is duplicated, maybe a dedicated file components/SelectOption.tsx could be more appropriate

m4theushw

I pushed a4110eb simplifying the way we choose which component to use for the options. I think we were duplicating too much code. What do you think now?

m4theushw · 2022-04-07T15:52:13Z

packages/grid/x-data-grid/src/components/cell/GridEditSingleSelectCell.tsx

@@ -12,17 +12,21 @@ import { useGridRootProps } from '../../hooks/utils/useGridRootProps';
 import { GridEditModes } from '../../models/gridEditRowModel';
 import { GridEvents } from '../../models/events/gridEvents';
 import { GridColDef, ValueOptions } from '../../models/colDef/gridColDef';
+import { getValueFromValueOptions } from './editCellUtils';


Could we reuse the functions from filterPanelUtils.ts?

kyeongsoosoo · 2022-04-07T17:46:42Z

It looks simpler than my first version.
Thanks for a better solution.

alexfauquette

I added a commit to change import helpers from the single select

Just spot a typo, otherwise it looks good

packages/grid/x-data-grid/src/components/panel/filterPanel/GridFilterInputValue.tsx

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

kyeongsoosoo force-pushed the filter-select branch from c39f5b1 to 756d28a Compare April 4, 2022 16:46

[DataGrid] Enable using MUI Select Component

632457b

kyeongsoosoo force-pushed the filter-select branch from 756d28a to 632457b Compare April 5, 2022 03:25

flaviendelangle requested a review from alexfauquette April 5, 2022 08:27

alexfauquette reviewed Apr 5, 2022

View reviewed changes

alexfauquette requested a review from m4theushw April 5, 2022 09:09

m4theushw reviewed Apr 5, 2022

View reviewed changes

m4theushw added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Apr 5, 2022

kyeongsoosoo added 2 commits April 6, 2022 02:15

use helper component for refactor

596ddb2

enable using native component in edit-single-select-cell

6caef46

alexfauquette reviewed Apr 6, 2022

View reviewed changes

alexfauquette requested a review from m4theushw April 6, 2022 10:38

simplify which component to use for options

a4110eb

m4theushw reviewed Apr 7, 2022

View reviewed changes

m4theushw requested a review from alexfauquette April 7, 2022 16:13

replace editCellUtils by filterPanelUtils

1e5d9c3

alexfauquette approved these changes Apr 8, 2022

View reviewed changes

packages/grid/x-data-grid/src/components/panel/filterPanel/GridFilterInputValue.tsx Show resolved Hide resolved

add missing prop

3be4afd

Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

m4theushw approved these changes Apr 8, 2022

View reviewed changes

m4theushw changed the title ~~[DataGrid] Enable using MUI Select Component in Filter Panel~~ [DataGrid] Enable using non-native Select in filter panel Apr 8, 2022

m4theushw merged commit a38c154 into mui:master Apr 11, 2022

alexfauquette mentioned this pull request Apr 12, 2022

[question] How to theme the singleSelect options dropdown #4305

Closed

2 tasks

m4theushw mentioned this pull request Jun 6, 2022

[data grid] FilterPanel column select menu is not customizable #5102

Open

2 tasks

alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022

[DataGrid] Enable using non-native Select in filter panel (mui#4361)

5f42c8d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Enable using non-native Select in filter panel #4361

[DataGrid] Enable using non-native Select in filter panel #4361

kyeongsoosoo commented Apr 4, 2022 •

edited by m4theushw

Loading

mui-bot commented Apr 4, 2022 •

edited

Loading

alexfauquette left a comment

alexfauquette Apr 5, 2022

m4theushw Apr 5, 2022

kyeongsoosoo Apr 5, 2022 •

edited

Loading

m4theushw left a comment

m4theushw Apr 5, 2022

kyeongsoosoo commented Apr 5, 2022

alexfauquette left a comment

alexfauquette Apr 6, 2022

m4theushw left a comment

m4theushw Apr 7, 2022

kyeongsoosoo commented Apr 7, 2022

alexfauquette left a comment

	if (process.env.NODE_ENV !== 'production') {
	if (!columnTypeWarnedOnce && !columnTypes[type]) {
	console.warn(
	[
	`MUI: The column type "${type}" you are using is not supported.`,
	`Column type "string" is being used instead.`,
	].join('\n'),
	);
	columnTypeWarnedOnce = true;
	}
	}

[DataGrid] Enable using non-native Select in filter panel #4361

[DataGrid] Enable using non-native Select in filter panel #4361

Conversation

kyeongsoosoo commented Apr 4, 2022 • edited by m4theushw Loading

Before

After

mui-bot commented Apr 4, 2022 • edited Loading

alexfauquette left a comment

Choose a reason for hiding this comment

alexfauquette Apr 5, 2022

Choose a reason for hiding this comment

m4theushw Apr 5, 2022

Choose a reason for hiding this comment

kyeongsoosoo Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

m4theushw left a comment

Choose a reason for hiding this comment

m4theushw Apr 5, 2022

Choose a reason for hiding this comment

kyeongsoosoo commented Apr 5, 2022

alexfauquette left a comment

Choose a reason for hiding this comment

alexfauquette Apr 6, 2022

Choose a reason for hiding this comment

m4theushw left a comment

Choose a reason for hiding this comment

m4theushw Apr 7, 2022

Choose a reason for hiding this comment

kyeongsoosoo commented Apr 7, 2022

alexfauquette left a comment

Choose a reason for hiding this comment

kyeongsoosoo commented Apr 4, 2022 •

edited by m4theushw

Loading

mui-bot commented Apr 4, 2022 •

edited

Loading

kyeongsoosoo Apr 5, 2022 •

edited

Loading