Skip to content
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

feat(BulkSelect demo): add bulk select demo with table and toolbar #3082

Merged
merged 7 commits into from Oct 22, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Oct 7, 2019

What: adds an interactive demo between table and toolbar for bulk select functionality.

Refer to issue: #2910

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 7, 2019

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 7, 2019

Codecov Report

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

Impacted file tree graph

@@           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
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.23% <ø> (ø) ⬆️
#patternfly4 68.11% <ø> (ø) ⬆️

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.

Copy link
Member

dlabrecq left a comment

After using the "select page (20 items)" on a couple pages, I found it a little odd that I could not deselect the page. If I want to deselect the current page, I either have to start over completely or deselect all 20 items in the page manually. That's a pain if I had already selected items on other pages.

It would be nice if the "select page (20 items)" turned into "deselect page (20 items)" after all items in the page have been selected.

Does the design include a deselect page feature?

@tlabaj tlabaj requested review from mcoker and mcarrano Oct 8, 2019
@tlabaj tlabaj self-assigned this Oct 8, 2019
Copy link
Member

mcarrano left a comment

This is looking good @kmcfaul but just one issue I see. The checkbox should also be interactive to shortcut opening the menu to select items. So if either none or some are selected and I click the checkbox, it will select all. If all are selected, select none. @katierik can you confirm that this is your expectation, also?

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Oct 8, 2019

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.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 9, 2019

@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.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 9, 2019

@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'.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Oct 9, 2019

@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.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 9, 2019

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 kmcfaul force-pushed the kmcfaul:table-bulk-select-demo branch from 9359f85 to c82f69b Oct 10, 2019
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 10, 2019

@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?

@katierik

This comment has been minimized.

Copy link

katierik commented Oct 10, 2019

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.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 14, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 14, 2019

@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?

@katierik

This comment has been minimized.

Copy link

katierik commented Oct 14, 2019

@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

@kmcfaul kmcfaul force-pushed the kmcfaul:table-bulk-select-demo branch from c82f69b to ae3419d Oct 14, 2019
@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 14, 2019

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

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 14, 2019

@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!

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 15, 2019

@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.

…ed up checkbox event handling s
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 15, 2019

@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.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 15, 2019

@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.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 15, 2019

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.

Copy link
Member

mcarrano left a comment

I just checked with @katierik and we are both good to leave the 'Select page' behavior as is. This looks ready to merge now from a UX standpoint.

@mcarrano mcarrano added ux approved and removed ux review labels Oct 15, 2019
fetch(page, perPage) {
this.setState({ loading: true });
fetch(`https://jsonplaceholder.typicode.com/posts?_page=${page}&_limit=${perPage}`)

This comment has been minimized.

Copy link
@redallen

redallen Oct 16, 2019

Contributor

Not a huge fan of being dependent on jsonplaceholder.typicode.com , but understandable for now.

Copy link
Member

seanforyou23 left a comment

A few small comments, overall it looks great! Nice job 👍

@kmcfaul kmcfaul dismissed stale reviews from redallen and mcarrano via c03deaa Oct 18, 2019
Copy link
Member

seanforyou23 left a comment

👍Nice work!

@redallen redallen merged commit 1f0defa into patternfly:master Oct 22, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.