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

Enable multi-select behavior #175

Merged
merged 1 commit into from
Feb 8, 2018
Merged

Enable multi-select behavior #175

merged 1 commit into from
Feb 8, 2018

Conversation

frosetti
Copy link
Contributor

@frosetti frosetti commented Feb 8, 2018

This project uses semver, please check the scope of this pr:

  • #none# - documentation fixes and/or test additions
  • #patch# - backwards-compatible bug fix
  • #minor# - adding functionality in a backwards-compatible manner
  • #major# - incompatible API change

CHANGELOG

  • Added disableDeselectAll. If true, clicking outside of the checkbox in a list item will no longer deselect all of the other items.

@notmessenger notmessenger self-requested a review February 8, 2018 20:37
Copy link
Contributor

@notmessenger notmessenger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frosetti Can you please provide a screen recording or a series of image captures elaborating on what your changelog description is describing? If I visit http://ciena-frost.github.io/ember-frost-list/#/ I can already select multiple items, can shift-click to do so, ctrl click, etc. I do not understand what the change in behavior is.

Can you also add a demonstration of this to the demo app so that others can see the behavior illustrated as well?

@TheOtherDude
Copy link
Contributor

To give context, we need this feature because we have a very large list and accidentally missing the select box is a huge pain for our users. This feature has been requested by Katie Parmeson and other people on the UX team.

@notmessenger
Copy link
Contributor

I'm not trying to capriciously block this but I still do not understand the difference in the behavior introduced in this PR and would still like to see what new behavior these changes provide.

@TheOtherDude
Copy link
Contributor

The changelog might be more clear as something like...

  • Added disableDeselectAll. If true, clicking outside of the checkbox in a list item will no longer deselect all of the other items.

@notmessenger
Copy link
Contributor

Only interacting with checkboxes:

  • can select individual boxes and they remain checked
    • can select individual boxes again and they become unchecked
  • can ctrl-click individual boxes and they remain checked
    • can ctrl-click individual boxes again and they become unchecked
  • can select a box and then shift-click another to select the range
    • can then deselect individual checkboxes

Only interacting with the rows (not the checkboxes):

  • Can make a checkbox be checked by selecting a row
  • Selecting another row deselects the previous checkbox and checks the new one
  • If selecting a row and shift-click another the entire range becomes checked
    • clicking another row then deselects all but the one just clicked on
  • ctrl-clicking various rows allows each successive row to be checked

What we should be doing is first synchronizing the behavior between what happens when interacting with the checkbox vs the rows - are they supposed to behave differently? Once that is squared away it may be that this PR isn't even needed.

@notmessenger notmessenger dismissed their stale review February 8, 2018 21:09

Have received an explanation of the change in behavior, though have a made a note that it is not how I believe we should approach this problem.

@cstolli
Copy link
Contributor

cstolli commented Feb 8, 2018

@notmessenger, Forrest convinced me there are valid use cases for both types of behavior, so an option makes sense.

@cstolli
Copy link
Contributor

cstolli commented Feb 8, 2018

👍

Approved with PullApprove

@cstolli cstolli merged commit f90fa83 into ciena-frost:master Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants