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
Add BulkController
support for shift+click behaviour
#10861
Add BulkController
support for shift+click behaviour
#10861
Conversation
Manage this branch in SquashTest this branch here: https://lb-feature5836-bulk-toggle-shi-k26mj.squash.io |
f142866
to
c8cace9
Compare
Rebased and fixed a few bugs encountered for edge cases. |
c8cace9
to
a70738a
Compare
Rebased |
a70738a
to
2ce47a9
Compare
dba9b8c
to
4f22059
Compare
4f22059
to
3665196
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working great @lb-, thank you! I’ve spotted minor things, which I’ll address myself as part of merging this.
@@ -1,5 +1,10 @@ | |||
import { Controller } from '@hotwired/stimulus'; | |||
|
|||
type ToggleAllOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m surprised this isn’t an interface
, but it works :)
<div id="bulk-container-multi" data-controller="w-bulk"> | ||
<input id="select-all-multi" type="checkbox" data-w-bulk-target="all" data-action="w-bulk#toggleAll"> | ||
<div id="checkboxes"> | ||
<input id="0" type="checkbox" data-w-bulk-target="item" data-action="w-bulk#toggle"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML element id
s normally can’t start with a number. Seems to work fine here but I’ll add a letter prefix just to be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call thank you, I think id
can be a number but CSS identifiers cannot start with a number.
toggleAll( | ||
event: CustomEvent<ToggleAllOptions> & { params?: ToggleAllOptions }, | ||
) { | ||
const { force = null } = { | ||
...event?.detail, | ||
...event?.params, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few things I’m confused about here:
- The types say
event
is aCustomEvent
, while the code below has an optional chaining as if this could be called without an event. But the code later accessesevent.target
anyway. - Unclear why we need to pass the data via
detail
in addition toparams
. I don’t see either of those two options used in Wagtail aside from the test suite. If we did use them, the use case fordetail
feels very small. force
seems like it doesn’t need a default value (but perhaps worth setting one for clarity?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @thibaudcolas - yes, I can see that this is a bit confusing. I would actually love some suggestions how to do this pattern better but let me explain the goals.
The purpose of this odd syntax is so that toggleAll
can be called a few ways.
- Internally from the class if needed
this.toggleAll()
- Via a simple Stimulus action
<input ... data-action"change->w-bulk#toggleAll" />
- Via dispatching some other event to this, also via a Stimulus action
<input ... data-action"some-event@document->w-bulk#toggleAll" />
However, if we want to use force
, we can now do this via the actions either with our custom event or with Stimulus action params.
<input ... data-action"change->w-bulk#toggleAll" data-w-bulk-force-param="true" />
OR
<input ... data-action"some-event@document->w-bulk#toggleAll" />
// JS
document.dispatchEvent(new CustomEvent({ detail: {force:true} }));
I could not find a nice way in TypeScript to reflect these three ways of calling, along with have a shared 'options' Type (to contain that force
could be X type). I would love any tips for this as this approach is super useful in many other controllers.
As for the force
defaulting to null
, I do not recall why, it's probably just a habit of mine to always think of the default as an intentional value. Instead of accidental undefined we are better off saying that this may not be passed in and if so it should default to something.
d0573a2
to
8717bd2
Compare
- Allow multiple selections or un-selections with shift+click - Pull out activeItems getter and instead use this as a method for select all checkboxes and individual checkboxes - Pull out ToggleAllOptions to a type and support this being passed in via a CustomEvent also, as per pattern being set up in other Controllers - Adjust unit test structure to be easier to work with different HTML values
8717bd2
to
8fa7ee4
Compare
Thanks Thibaud, let me know if you would like to see any improvement to this code as a follow up PR later. |
activeItems
getter and instead use this as a method for select all checkboxes and individual checkboxesadditional details for testing this change
Recording
screencast-bulk-actions-shift-click-2023.09.06-07_54_23.webm
Context
This PR builds on the work done in #10292 & #10613 and sets up the Stimulus controller to be closer to what is needed for the full 'bulk actions' checkbox support. This PR will be one of the final pieces before that work can start.