improvement(Table): implement indeterminate checkbox state by michpetrov · Pull Request #3909 · patternfly/patternfly-react

michpetrov · 2020-03-13T12:25:00Z

What: Closes #3648

Added indeterminate state; this cannot be achieved simply through HTML attributes, hence the use of hooks. Left in the allRowsSelected prop in case it's used somewhere but it's made redundant with this change.

(Kinda new to React, please correct me if I went against conventions)

patternfly-build · 2020-03-13T12:32:24Z

PF4 preview: https://patternfly-react-pr-3909.surge.sh
A11y report: https://patternfly-react-pr-3909-coverage.surge.sh

michpetrov · 2020-03-16T18:49:47Z

Okay, I think I fixed the tests; one question regarding Selectable table with selected expandable row, is the purpose of the test to simply check the presence of a prop, or to check that the check-all checkbox is indeed checked? Since those two are somewhat independent.

Should I add test for "some rows are selected"?

seanforyou23

Nice work! I think unfortunately we don't yet utilize hooks in patternfly react due to legacy react version support. As much as I'd love to change this, I'd have to ask for the moment - can we refactor this to a class-based solution?

Another thing worth considering is the usage of the name amount - might it better be named cardinalityType? Interested to hear other opinions on this.

michpetrov · 2020-03-20T18:37:25Z

@seanforyou23 I see useRef being used in TextInput but I can refactor the component.

seanforyou23 · 2020-03-20T21:36:06Z

Thx for pointing this out! Checking into what we can do, keep you posted!

mcarrano

@kriker can you help me out with this one? For bulk selection in a table, I believe the way we said it should work is for selecting the checkbox to clear selection (not select all) when you start with partial selection. That's what I remember but I can't find it written down anywhere. Can you confirm? It's currently working the opposite here.

tlabaj · 2020-03-25T14:16:57Z

@michpetrov useRef is only used int the TextInput example to show how a PF consumer might be able to use it. We do not yet utilize hooks in our PF library as @seanforyou23 mentioned above.

michpetrov · 2020-03-26T13:06:33Z

Refactored the component, rebased the branch. There is still the one test failing, I don't know what it is testing so I don't know what to fix.

seanforyou23 · 2020-04-14T13:34:54Z

@michpetrov I'll take a look later today and see if I can help find out what gives with the test. Thanks for the refactoring!

seanforyou23 · 2020-04-15T15:05:48Z

Hi there @michpetrov - just a heads up I sent a PR to your working branch - seems to be related to this commit bd3f49f

Looking into the issue I think I found some problems that while unrelated to this work item - is impacted by it. I'm going to see if I can isolate the unrelated problem (described in the cypress test) without these changes and maybe that will fix the problem without having to do anything further here. It's just a guess at this point though, so feel free to proceed investigating on your end with the demo I provided in the mean time. Hope it helps, keep you posted!

seanforyou23 · 2020-04-15T18:41:59Z

  compact = 'compact'
 }

+export enum SelectedRowsAmount {


Do you think we could assign strings to represent these enum values? I think using the enum indices is really elegant but makes it a little difficult to debug when comparing against the resulting markup. In other words I thought data-rowsamount="1" meant there was a single row selected instead of it referring to "some".

seanforyou23 · 2020-04-15T18:46:12Z

 ) => {
  const {
-    extraParams: { onSelect, allRowsSelected, rowLabeledBy = 'simple-node' }
+    extraParams: { onSelect, allRowsSelected, selectedRowsAmount, rowLabeledBy = 'simple-node' }


If we no longer need allRowsSelected, can we remove it?

I'll remove it, I kept it in in case something depended on it.

seanforyou23 · 2020-04-15T18:54:13Z

          'aria-labelledby': rowLabeledBy + rowIndex
        }
      : {
-          checked: allRowsSelected,


Seems like this may be what's causing that test to fail.

michpetrov · 2020-06-04T13:01:35Z

@seanforyou23 I've changed the failing test and added your cypress tests

dlabrecq

Looking... Can you please rebase to fix the merge conflict?

michpetrov · 2020-06-22T16:42:20Z

@dlabrecq rebased

fix patternfly#3648

dlabrecq

After reviewing the code, I realized we already have a "Bulk select table" feature, which performs the same functionality and more. It shows how many items are selected, it selects one or more pages at a time, etc.

Do we want to support both features?

See http://patternfly-react.surge.sh/documentation/react/demos/bulkselecttable

Regarding the example itself, I found the behavior a little confusing. I expect the "select all" toggle to show the same behavior as if I manually clicked on each checkbox. That is, similar to the "Bulk select table" example.

When I click the "select all" toggle, all checkboxes are selected, including the disabled one.

When I select each checkbox manually, the "select all" toggle is shown in the indeterminate state. I presume that's because one of the checkboxes is disabled and cannot be selected.

Now if I click on the "select all" toggle, then deselect each checkbox manually, the "select all" toggle remains in an indeterminate state. I cannot deselect the remaining checkbox because it's disabled. I have to select all again, then deselect all to remove the selection.

mcarrano · 2020-06-24T21:08:35Z

I think that @dlabrecq makes some good points. This show work in the same way as the bulk selection from the toolbar. The only reason to still include this was for cases where you might want to use a table without a toolbar.

Venefilyn · 2020-07-18T09:31:10Z

I can see how the bulk selection could work if you have a toolbar, but at the same time it will increase the size of the toolbar, which might already be full enough. Unless the select all functionality in the Table will be deprecated I believe having a select-all checkbox that can be indeterminate makes sense

I've got a question: If the user would never select all due to the Table contents but you still want to display that something has been selected, how would you go about doing that without a the select-all or bulk select component?

stale · 2020-09-20T03:22:25Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

michpetrov force-pushed the selectable-table branch 3 times, most recently from a25602f to 05da5e8 Compare March 16, 2020 18:43

tlabaj requested review from evwilkin, kmcfaul and seanforyou23 and removed request for evwilkin March 19, 2020 16:05

seanforyou23 reviewed Mar 19, 2020

View reviewed changes

tlabaj added the ux review label Mar 23, 2020

tlabaj requested a review from mcarrano March 23, 2020 17:11

mcarrano reviewed Mar 23, 2020

View reviewed changes

michpetrov force-pushed the selectable-table branch from 05da5e8 to 2461b02 Compare March 26, 2020 12:52

jenny-s51 self-requested a review March 30, 2020 19:31

seanforyou23 mentioned this pull request Apr 15, 2020

[test] add some tests to illustrate the problem michpetrov/patternfly-react#1

Open

seanforyou23 reviewed Apr 15, 2020

View reviewed changes

michpetrov force-pushed the selectable-table branch from 2461b02 to 2c1000f Compare April 17, 2020 16:16

Venefilyn mentioned this pull request May 17, 2020

Add indeterminate checkbox to table header #3648

Closed

michpetrov force-pushed the selectable-table branch from 2c1000f to 24173b6 Compare May 25, 2020 19:26

michpetrov force-pushed the selectable-table branch 3 times, most recently from 3d31538 to 3220c67 Compare June 3, 2020 17:14

dlabrecq reviewed Jun 19, 2020

View reviewed changes

michpetrov force-pushed the selectable-table branch from 3220c67 to 97474e0 Compare June 22, 2020 16:41

michpetrov and others added 2 commits June 22, 2020 19:00

improvement(Table): implement indeterminate checkbox state

fd12831

fix patternfly#3648

[test] add some tests to illustrate the problem

fb8adb4

michpetrov force-pushed the selectable-table branch from 97474e0 to 1d8b828 Compare June 22, 2020 17:01

refactor(demo-app-ts/tableSelectableExpandableDemo): update test files

d7f1dad

michpetrov force-pushed the selectable-table branch from 1d8b828 to d7f1dad Compare June 23, 2020 12:11

dlabrecq requested changes Jun 24, 2020

View reviewed changes

stale Bot added the wontfix label Sep 20, 2020

stale Bot closed this Oct 4, 2020

rhamilto mentioned this pull request May 1, 2026

Table: Add indeterminate checkbox state support for select-all header #12404

Open

Conversation

michpetrov commented Mar 13, 2020

Uh oh!

patternfly-build commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michpetrov commented Mar 16, 2020

Uh oh!

seanforyou23 left a comment

Choose a reason for hiding this comment

Uh oh!

michpetrov commented Mar 20, 2020

Uh oh!

seanforyou23 commented Mar 20, 2020

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Mar 25, 2020

Uh oh!

michpetrov commented Mar 26, 2020

Uh oh!

seanforyou23 commented Apr 14, 2020

Uh oh!

seanforyou23 commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanforyou23 Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanforyou23 Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

michpetrov Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

seanforyou23 Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

michpetrov commented Jun 4, 2020

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

michpetrov commented Jun 22, 2020

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano commented Jun 24, 2020

Uh oh!

Venefilyn commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale Bot commented Sep 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

patternfly-build commented Mar 13, 2020 •

edited

Loading

seanforyou23 commented Apr 15, 2020 •

edited

Loading

Venefilyn commented Jul 18, 2020 •

edited

Loading