[DataGrid] Fix checkbox selection issue #285

dtassone · 2020-09-14T15:39:49Z

oliviertassinari

I have tried to think at how we could test this fix. I can't think of a simple answer. Maybe it's one of these rare cases where we can ignore it.

oliviertassinari · 2020-09-14T18:30:47Z

packages/grid/x-grid-modules/src/components/checkbox-renderer.tsx

+    setChecked(isCurrentChecked);
+    setIndeterminate(isCurrentIndeterminate);


Why have the logic in an effect? Would it be simpler to pull the value from the state at render time, with the context?

How will we handle the controllable API? #246: https://next.material-ui.com/components/data-grid/selection/#controlled-selection

All these derived states should be computable at render time which the naming of the methods indicated. E.g. get* implies to me that there are no side-effects which means it's safe to call at render time. Then getDerivedStateFromProps would apply which has a documented pattern for hooks: https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops

oliviertassinari · 2020-09-14T18:43:39Z

packages/grid/x-grid-modules/src/components/checkbox-renderer.tsx

@@ -17,7 +17,7 @@ export const HeaderCheckbox: React.FC<ColParams> = React.memo(({ api }) => {
      const isAllSelected =
        api.getAllRowIds().length === event.rows.length && event.rows.length > 0;
      const hasNoneSelected = event.rows.length === 0;
-      setChecked(isAllSelected || !hasNoneSelected);
+      setChecked(isAllSelected && !hasNoneSelected);


Are you sure?

vs.

(Google Ads)

For me, checked is when all checkbox are checked and none are unchecked
But up to you, I can change it

It makes me think of mui/material-ui#20476. No strong preferences. It could simply surface that the intermediate state should always have the same visual look no matter the checked state. At least it's what bootstrap and ant design do.

@eps1lon any tips on the matter?

What's the question here? If this is about color then the mixed state should have the same color as the checked state. At least that's how our Checkbox behaves so if it behaves different here then there should be a rationale that doesn't apply to the general purpose use case.

Also note that the color doesn't really matter anyway because of SC 1.4.1 in WCAG 2.x. If there's a difference between checked+indeterminate and checked+determinate then it needs to be encoded in something other than color as well. Though ARIA does not support both of these states but rather a single "mixed" state.

OK awesome. I think that it perfectly answer the question, when indeterminate is set:

It doesn't matter if checked is true or false, it's in a mixed state.

The color shouldn't change based on the checked state, if it does, it could incentive people to break 1.4.1 in in WCAG 2.1.

So as the next step, I propose:

On the main repository side, we update the CSS so indeterminate unchecked has the primary color.

Here, we keep the previous logic as the fix will land on v5 and the grid depends on v4, we need the primary color.

On the main repository side, we update the CSS so indeterminate unchecked has the primary color.

@dtassone Do you want to use this issue as an opportunity to make your first contribution to the core? :)

eps1lon · 2020-09-15T13:31:21Z

packages/grid/x-grid-modules/src/components/checkbox-renderer.tsx

+  const isCurrentChecked = selectedRows.length === allRowsCount || isCurrentIndeterminate;
+
+  const [isChecked, setChecked] = React.useState(isCurrentChecked);
+  const [isIndeterminate, setIndeterminate] = React.useState(isCurrentIndeterminate);


Does api not change if we select a different row? Seems very brittle to have individual components duplicate the logic of which row is considered checked and which isn't.

this is just the header checkbox

It always starts out with "just this small component". If we have the choice between robust and brittle we might as well use robust. I guess api is not changing if the underlying data changes? How are "subscribers" usually notified when the data changes?

No Api is not changing. They can use an event handler to get latest data.

oliviertassinari · 2020-09-15T15:01:07Z

packages/grid/x-grid-modules/src/components/checkbox-renderer.tsx

-export const HeaderCheckbox: React.FC<ColParams> = React.memo(({ api }) => {
-  const [isChecked, setChecked] = React.useState(false);
-  const [isIndeterminate, setIndeterminate] = React.useState(false);
+export const HeaderCheckbox: React.FC<ColParams> = ({ api }) => {


We do no longer need the memo?

Would it make sense to pull the api from the context here?

This is just an implementation of the col prop renderHeader.
So we pass api as a prop so it's easier for the user.
Yes users could use the context as well.

Fix mui#283 selection issue

40904d0

dtassone changed the title ~~Fix #283 selection issue~~ [DataGrid] Fix #283 selection issue Sep 14, 2020

dtassone requested a review from oliviertassinari September 14, 2020 15:42

dtassone changed the title ~~[DataGrid] Fix #283 selection issue~~ [DataGrid] Fix #283 checkbox selection issue Sep 14, 2020

oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Sep 14, 2020

oliviertassinari changed the title ~~[DataGrid] Fix #283 checkbox selection issue~~ [DataGrid] Fix checkbox selection issue Sep 14, 2020

oliviertassinari reviewed Sep 14, 2020

View reviewed changes

fix checkbox intermediate state

5a73c24

dtassone requested a review from oliviertassinari September 15, 2020 13:15

eps1lon reviewed Sep 15, 2020

View reviewed changes

oliviertassinari approved these changes Sep 15, 2020

View reviewed changes

oliviertassinari reviewed Sep 15, 2020

View reviewed changes

dtassone merged commit 0d47c38 into mui:master Sep 15, 2020

oliviertassinari mentioned this pull request Sep 17, 2020

[Checkbox] Improve indeterminate UI mui/material-ui#22635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Fix checkbox selection issue #285

[DataGrid] Fix checkbox selection issue #285

dtassone commented Sep 14, 2020 •

edited by oliviertassinari

oliviertassinari left a comment

oliviertassinari Sep 14, 2020

eps1lon Sep 15, 2020 •

edited

oliviertassinari Sep 14, 2020 •

edited

dtassone Sep 15, 2020

oliviertassinari Sep 15, 2020 •

edited

eps1lon Sep 15, 2020

oliviertassinari Sep 15, 2020 •

edited

oliviertassinari Sep 15, 2020

eps1lon Sep 15, 2020

dtassone Sep 15, 2020

eps1lon Sep 15, 2020

dtassone Sep 15, 2020

oliviertassinari Sep 15, 2020

oliviertassinari Sep 15, 2020

dtassone Sep 15, 2020

		setChecked(isCurrentChecked);
		setIndeterminate(isCurrentIndeterminate);

[DataGrid] Fix checkbox selection issue #285

[DataGrid] Fix checkbox selection issue #285

Conversation

dtassone commented Sep 14, 2020 • edited by oliviertassinari

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon Sep 15, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari Sep 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Sep 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Sep 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtassone commented Sep 14, 2020 •

edited by oliviertassinari

eps1lon Sep 15, 2020 •

edited

oliviertassinari Sep 14, 2020 •

edited

oliviertassinari Sep 15, 2020 •

edited

oliviertassinari Sep 15, 2020 •

edited