PatternFly-React preview: https://patternfly-react-pr-3082.surge.sh

Codecov Report

Merging #3082 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3082   +/-   ##
=======================================
  Coverage   69.02%   69.02%           
=======================================
  Files         858      858           
  Lines       23535    23535           
  Branches     1877     1877           
=======================================
  Hits        16246    16246           
  Misses       6336     6336           
  Partials      953      953

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55e9654...8265c67. Read the comment docs.

Besides the interaction @mcarrano mentioned, there seems to be a bug where if you select all on the page, you get the indeterminate state checkbox. However if you select all, then select the page, you're left with the checked checkbox. I would expect the checkbox to be the same in those 2 scenarios.

@mcarrano Should the 'select page' button in the dropdown behave as a toggle for the page (deselecting, if page is selected)? Per dlabreq's feedback.

@mcarrano I will update the checkbox to act as a 'select all' toggle

@mcoker If you select all, and select the page, the demo currently checks for unchecked rows and checks them, adding them to the selected count/state. The total number of items doesn't change, all are still selected, so the check mark should be correct. Unless the behavior is meant to be 'when the page is all selected, unselect the whole page'.

@kmcfaul ah gotcha! I misunderstood, I thought if you went from "select all" to "select page", it would deselect anything that wasn't on the current page.

That's a good point though @mcoker, it could work like that and make sense. How do we want this 'select page' interaction @mcarrano?

@kmcfaul a couple of comments...

1- The checkbox now works as expected. Thanks for fixing that.
2- I don't think I'd expect clicking 'Select page' when all are selected would deselect items on other pages, but it's a reasonable question. Let's leave this alone for now.
3- @dlabrecq makes an interesting point about having 'Select page' toggle to 'Deselect page' when on a page that is already selected. The only think that's tricky is what happens if I just deselect one item on that page, does it stay deselect? Toggle back? It would require tracking the selection state of a page. @katierik what do you think.
4- I seem to have com across another bug. If I select 2 pages, i.e. 40 items are selected, and then move to the next page, suddenly all 100 items become selected. @kmcfaul can you try to reproduce?

In the POC for this we found that when anything was selected, it made the most sense to deselect is the user clicked on the bulk select box again (vs the original design/this implementation that clicking the partially selected selects more).
I suspect that change would account for part of the "select page" confusion.

@mcarrano I've been unable to reproduce the selection bug. Selecting the two pages manually or with two sets of 'select page' and then clicking on the next page button doesn't select all for me. Is it the next page button that is triggering the selection for you or is it another button?

@katierik So the interaction for the select page should be: when there are no selections, select the whole page; when there are some selections / when the whole page is selected, deselect the whole page?

@kmcfaul yes!
This gif may help (it doesn't show the clicks though :/)
http://www.giphy.com/gifs/cn2fWChDzdmzZVnKKS
1 - Click the toolbar select all - whole list is selected
2 - Click the first two items, then click the toolbar select all about - whole list deselected

This commit changes the top level toggle to clear selections when anything is selected (previously, selected everything first and only would deselect if everything was selected). That should match the gif.

Should the 'select page' button switch between select and deselect for just the page though? I'm still confused if that should go in or not. @katierik

@mcarrano I've been unable to reproduce the selection bug. Selecting the two pages manually or with two sets of 'select page' and then clicking on the next page button doesn't select all for me. Is it the next page button that is triggering the selection for you or is it another button?

@kmcfaul I did some more detective work on this and here's what seems to be happening. If I'm careful to click on the toggle in the split button it works fine, but if I accidentally click on the text, it's the same as clicking the checkbox. Can we make it so the text is not active?

Also, I think it would help the demo if we can drop a pagination component in the top toolbar as well as the footer. It would just be easier to know what page I'm on. Thanks!

@mcarrano The text fires off the click/change events natively as it is a label for the checkbox. I would have to restructure the DropdownToggleCheckbox component a bit to disable the text firing off these events. Should I go ahead and include that in this PR, or would this be a separate issue?

I will add the extra pagination location.

@kmcfaul Yes, I think we will want to do that restructuring. Normally we do want to have a checkbox label toggle the state of a checkbox, but in this case the text does not label the checkbox exactly, so it becomes a usability problem. I would be OK if clicking on the text either did nothing or if it opened the dropdown menu. Will this also require a core change? There may also be an accessibility consideration here. @jessiehuff thoughts?

As for whether to combine with this PR or open a new issue, I will defer to @tlabaj . If you need me to open a separate issue for this, I can do that.

@mcarrano I found an alternative, taking the text out of the checkbox component, the spacing is a little wider but it should behave as intended. I also took out the behavior where clicking the checkbox also opens the dropdown, but if that should be preset I can add it back in.

Excellent @kmcfaul ! I actually like the wider spacing as it makes it clear that the text and checkbox are not functionally connected. I think the only pending question is whether the 'select page' should toggle to 'deselect page'. Let me connect with @katierik today and we'll give you an answer.