feat: maximum selectable rows on the Datatable #2451
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @johnvente! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
mphilbrick211
added
the
needs test run
johnvente
changed the title
There was a problem hiding this comment.
Appreciate the contribution, @johnvente!
Limiting the number of selected rows in DataTable is definitely a use case we've faced at 2U/edX as well in at least one micro-frontend.
That said, I might recommend bringing your solution to the weekly Open edX Paragon Working Group meeting for discussion/review and/or posting in Slack as well, given this also has some design implications around the approach/mechanism to limit the number of selections.
| const toggleRowsSelectedProps = useMemo( | ||
| () => { | ||
| // determine if this selection is for an individual page or the entire table | ||
| const getToggleRowsSelectedProps = page ? getToggleAllPageRowsSelectedProps : getToggleAllRowsSelectedProps; | ||
| return getToggleRowsSelectedProps(); | ||
| return isSelectable && maxSelectableRows ? { ...getToggleRowsSelectedProps(), disabled: 'disabled', onChange: () => {} } : getToggleRowsSelectedProps(); |
There was a problem hiding this comment.
[suggestion] If the goal if to ultimately get disabled attribute as part of the updatedProps below via modifying toggleRowsSelectedProps, I might recommend instead to add the new disabled prop directly to the CheckboxControl component below as opposed to extending react-table's internals with getToggleRowsSelectedProps().
There was a problem hiding this comment.
Yep, that is the goal to disable the CheckboxControl but also if the checkbox is marked not to allow doing anything because someone can modify disabled attribute in the DOM prop and try to mark the checkbox
src/DataTable/index.jsx
Outdated
| Note: We override the `toggleAllRowsSelected` action from react-table | ||
| because we need to reassign the `selectedRowIds` property depending on | ||
| the size of the maximum rows selected. |
There was a problem hiding this comment.
[clarification] I'm curious on the approach here. I'm not sure we should naively uncheck the previous selected row on the user's behalf when selecting another row after the user has exceeded the number of selections specified via the maxSelectedRows prop.
We have a similar use case in frontend-app-admin-portal, where we make the selection checkbox on each row disabled as well when the user has reached the maxSelectedRows as configured, though we have a custom MFE-specific implementation, e.g.: https://github.com/openedx/frontend-app-admin-portal/blob/71de4bc001eb9cf09e9118c0d495b250af9df2ee/src/components/ContentHighlights/HighlightStepper/SelectContentSelectionCheckbox.jsx#L21
Our approach in the above use case leaves the choice up to the user how they want to handle reaching the max number of selected rows, e.g. perhaps they want to uncheck the last checked row but they might also want to choose a lesser priority row from an earlier selection, too. Your current implementation prevents the user from making this intentional choice, which I feel is a usability issue.
I might recommend a pattern more alike to frontend-app-admin-portal's use case where the row's selection checkbox becomes disabled preventing the number of selected rows from exceeding the maxSelectedRows prop.
There was a problem hiding this comment.
Why could it be a usability issue? if the user wants to chose the previews selected row may check it again and uncheck the checkboxes that he doesn't want to select, even doing is the last checkbox that will be changed it will be just that one. I think that disable the other rows can be aggressive for the user, I mean can think that is not possible to check them after the selection
There was a problem hiding this comment.
@johnvente. I spoke with the designer responsible for the existing pattern for DataTable to limit the number of maximum selected rows. We both feel it's preferable to prevent going beyond the maximum configured by disabling selections once the user has reached the maximum.
The rationale for this decision is due to the usability issue of the proposed approach assuming the user will choose the de-select the previously selected row, which is not an assumption we can make. If that previously selected row was not a row the user would have de-selected on their own, they would now need to re-select the row that was programmatically de-selected. We feel this is not efficient for the user and likely to cause confusion; the user should be able to make an intentional choice of which row to deselect.
Further, while DataTable should enforce disabling of selection checkboxes, we also believe there is a separate design task to think through how to best communicate the maximum number of rows the user can select as an official part of the DataTable UI. For example, in our current use case, we have some micro-copy nearby the DataTable to explain to the user that "up to 12 rows may be selected." The design task would help understand whether such messaging be a generic, consistent pattern for all use cases.
Given the follow-up design task to explore this pattern, I recommend the following next steps:
- Implement
maxSelectedRowsprop to disable selection checkboxes once a configured maximum has been reached. For the "Select all" checkbox in the table header, I feel it would be preferable to simply not include a checkbox in the table header rather than having its checkbox disabled.- Perhaps we could conditionally hide the "Select all" checkbox when
maxSelectedRows=trueand the number of selected rows === number of items in the table (i.e.,itemCount)? 🤔
- Perhaps we could conditionally hide the "Select all" checkbox when
- [consider] Understand whether consumers of the
DataTablecomponent have the ability to get at the current number of selected rows outside ofDataTablechildren (i.e., external to theDataTableContext).- If not, perhaps introducing a callback function prop (e.g.,
onRowSelectedoronMaxSelectedRows) which would get called byDataTablewhen the user selects a row, or reaches the configured maximum number of rows. Using such props could allow consumers ofDataTableto render custom messaging where appropriate if/when the user reaches the maximum number of selected rows.
- If not, perhaps introducing a callback function prop (e.g.,
There was a problem hiding this comment.
Hey @adamstankiewicz! Thanks so much for being aware and giving me that feedback. I will check this ASAP
e0d
removed
the
needs test run
johnvente
changed the title
@johnvente Netlify automatically adds a comment to your PR about it's deploy preview status and URL, e.g.: #2451 (comment) This URL is the Paragon docs site running the latest commit in your PR. It's usually helpful to leave a (direct) link such as https://deploy-preview-2451--paragon-openedx.netlify.app/components/datatable/ to ease the review process a bit. |
It's good heard that at 2U/edX have had the same case use. Last week I couldn't be in the meet but this week I will carry my solution to the weekly Open edX Paragon Working Group |
Thanks a lot 👍 |
johnvente
force-pushed
the
feat-max-selectable-rows
branch
2 times, most recently
from
460a3c4
to
8b64e63
Compare
|
Hi @adamstankiewicz! It's been a while since the last update, but I've done the changes with the feedback that you give me. Now it looks better 💪 |
|
Hi @adamstankiewicz! Would you mind enabling tests on this? They bounced back to needing approval. |
mphilbrick211
added
the
needs test run
adamstankiewicz
removed
the
needs test run
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2451 +/- ##
==========================================
+ Coverage 91.38% 91.70% +0.31%
==========================================
Files 234 236 +2
Lines 4157 4219 +62
Branches 1001 1021 +20
==========================================
+ Hits 3799 3869 +70
+ Misses 351 346 -5
+ Partials 7 4 -3
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@johnvente Looking good; left some additional comments based on your changes. It's close! 😄
| @@ -21,7 +23,7 @@ export const selectColumn = { | |||
| ); | |||
| const updatedProps = useConvertIndeterminateProp(toggleRowsSelectedProps); | |||
|
|
|||
| return ( | |||
| return isSelectable && maxSelectedRows ? null : ( | |||
There was a problem hiding this comment.
Would it be possible to split this logic up into explicit if statements? Might be a bit more readable. E.g.,
if (isSelectable && maxSelectedRows) {
return null;
}
return (
...
);| const { index } = row; | ||
| const isRowSelected = index in selectedRowIds; | ||
| const selectedRowsLength = Object.keys(selectedRowIds).length; | ||
| const hasMaxSelectedRows = maxSelectedRows && maxSelectedRows === selectedRowsLength; |
There was a problem hiding this comment.
[sanity check] Does this conditional logic handle when maxSelectedRows === 0?
There was a problem hiding this comment.
It doesn't because if maxSelectedRows is equal to 0 in this case would be false, should we handle it? It would be odd that the rows are selectable but I can not select one of them 😅 don't you think? I'm wondering if that would be a case use
There was a problem hiding this comment.
If maxSelectedRows is 0, hasMaxSelectedRows would be 0, not false (which goes against the implied boolean by the variable name).
should we handle it? It would be odd that the rows are selectable but I can not select one of them 😅 don't you think? I'm wondering if that would be a case use
You're right that it's unlikely to be a use case. However, I still feel the component should adhere to the 0 if passed by the consumer. Otherwise, it's not really "correct" (the consumer is telling the component 0 rows should be selected lol). Allowing selections with maxSelectedRows=0 doesn't feel great imho 😄 What if that 0 is intentional?
|
@johnvente Also note, this commitlint CI check is failing. It's possible the commitlint check might not like the parentheses in the commit message? 
|
|
Hello @adamstankiewicz! I've done the changes I'm having account when |
Hi @adamstankiewicz we're almost done 🚀 I've pushed some test cases I'll be pending to any change 👯 |
mphilbrick211
added
the
waiting on author
johnvente
force-pushed
the
feat-max-selectable-rows
branch
3 times, most recently
from
728f047
to
fd2ab83
Compare
There was a problem hiding this comment.
Testing this component in the playground, I noticed some odd behavior on the onMaxSelectedRows prop. (see video)
Test Case 1: User reaches max selected rows, where all selected rows are on the same pagination page of the datatable, and prop function executes.
Output: For every k number of selected rows where k = maxSelectedRows, the function executes k number of times.
Test Case 2: User reaches max selected rows where n number of selected rows one paginated page of the datatable, and k number of selected rows on a different page of the datatable, where n + k = maxSelectedRows.
Output: If you are on the paginated page of the datatable and the final selected row the user selects equals the max selected rows, then if the user is on the page with n selected rows and reaches the maximum number selected rows, then n number of calls will be made based on the function in onMaxSelectedRows.
Ex. maxSelectedRows = 4
Page 1: 1 selected item
Page 2: 3 selected items
if final selected item results in maxSelectedRows is page 1, onMaxSelectedRows will run once.
if final selected item results in maxSelectedRows is page 2, onMaxSelectedRows will run
3 times.
Screen.Recording.2023-08-07.at.10.22.44.AM.mov
Probably this may be related to useEffect dependences I'll review it |
johnvente
changed the title
johnvente
force-pushed
the
feat-max-selectable-rows
branch
from
fd2ab83
to
1de1586
Compare
Hey @brobro10000 I've changed the behavior for that, please check if now works as expected |
|
Hi @adamstankiewicz and @brobro10000! Would one of you be able to re-enable the tests? They bounced back to needing approval to run. |
mphilbrick211
added
needs test run
There was a problem hiding this comment.
Just clicked around a bit on the preview site and this looks like it's working really well! @johnvente is there any chance you could add an example to https://github.com/openedx/paragon/blob/master/src/DataTable/README.md demonstrating this functionality? (@adamstankiewicz if you feel like that's not something we want/it would just clutter up the docs page feel free to veto this idea)
brian-smith-tcril
removed
the
needs test run
Hi @brian-smith-tcril! Absolutely, I will add an example for this. Thanks for the suggestion! 💪 |
|
@brian-smith-tcril @johnvente Sure, feel free to add an example. The overall docs for |
|
Hey @brian-smith-tcril @adamstankiewicz! I've included an example for these new props. Any change or suggestion will be very welcome |
mphilbrick211
added
the
needs test run
|
Hi @adamstankiewicz and @brian-smith-tcril! Would one of you mind enabling the checks again? |
| @@ -1569,4 +1569,201 @@ You can create your own cell content by passing the `Cell` property to a specifi | |||
| </DataTable> | |||
| ); | |||
| } | |||
|
|
|||
| ``` | |||
| ## maxSelectedRows and onMaxSelectedRows props | |||
src/DataTable/README.md
Outdated
| ``` | ||
| #### example with react-intl |
There was a problem hiding this comment.
@johnvente What is the intent of this example with react-intl? I'm not quite sure what value it provides to consumers. What is this example trying to demonstrate for consumers? My two cents is that this particular example is unnecessary.
Also, as is, it results in a JS error within the example code block preview:
There was a problem hiding this comment.
@adamstankiewicz Probably this is unnecessary I'll remove it
There was a problem hiding this comment.
@adamstankiewicz done!
adamstankiewicz
removed
the
needs test run
|
@johnvente 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
🎉 This PR is included in version 21.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
@johnvente Thanks for your contribution! As the semantic-release bot commented above, your changes have been released on v21.1.0. |
Description
When we pass the isSelectable prop to the Datatable component, we can select any quantity of rows: all, one, or any other number we prefer. With the new maxSelectableRows prop, we can specify the maximum number of selectable rows and handle it more effectively
Demo
Deploy Preview
https://deploy-preview-2451--paragon-openedx.netlify.app/components/datatable/
Merge Checklist
exampleapp?wittjeffandadamstankiewiczas reviewers on this PR.Post-merge Checklist