-
-
Notifications
You must be signed in to change notification settings - Fork 531
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 selection across different pages with pagination="remote"
and selectable="checkbox"
#5889
Conversation
43a354a
to
fbc81e8
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5889 +/- ##
==========================================
- Coverage 84.41% 84.15% -0.26%
==========================================
Files 291 291
Lines 42655 42764 +109
==========================================
- Hits 36007 35988 -19
- Misses 6648 6776 +128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dd3f17a
to
c10d7bc
Compare
pagination="remote"
and selectable="checkbox
pagination="remote"
and selectable="checkbox
pagination="remote"
and selectable="checkbox"
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've left some comments. I have noticed that when selecting all the rows with the tickbox that is in the header, it selects the rows of the displayed page only. The behavior with local pagination is to select all the rows. Is that on purpose?
Very happy to see these many UI tests! 👍
|
||
event_name = 'selection-change' | ||
|
||
def __init__(self, model, indices, selected): |
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.
Instead of indices, selected
, how about having selected, deselected
, each one of the latter being a list (empty or not) of indices? Asking since it wasn't first obvious to me what selected
would hold.
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 am not the biggest fan of the names I have chosen.
Your suggestion could also be confusing since the indices are only updated ones and not a full list of selected/deselected indices.
@philippjfr, any preferences?
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.
No real preference, a comment to clarify is probably more important than the naming here since these events won't be user facing.
@hoxbro My main question is whether the title is still accurate? The idea was to support cross-page selection for all selection modes, not just the "checkbox" mode, but I'd be happy if your answer was that you will follow up in another PR. |
Not more than that was how it manifested itself when clicking on the top tickbar. It made sense to me that it would only select the current page, as this is how it works in apps like Gmail.
The title should still be accurate as Don't worry; I will add this in another PR 🙂 |
Fixes #4367
screenrecord-2023-11-17_17.17.53.mp4