TD-3359 TransferList update to have combined search and check functionality #1035
Conversation
…ity and added one more test
There was a problem hiding this comment.
Functionality of the component is as described. I suggest adding a few more tests, creating a common function to deal with the selection logic.
Also I have left a general question asking why are we exposing the onChange callback in a prop? Shouldn't this logic be internal and not up to the consumer of the component to decide?
There was a problem hiding this comment.
Shouldn't you be targeting a release branch instead of the main? If so, then the change in the package.json should most likely be done in the release branch.
| const handleCheckAllTarget = () => { | ||
| // If all target items are checked, uncheck them | ||
| // Otherwise, check all target items | ||
| allTargetItemsChecked | ||
| ? setChecked( | ||
| checked.filter(item => !targetItemsToTransfer.includes(item)) | ||
| ) | ||
| : setChecked([ | ||
| ...checked.filter(item => !targetItemsToTransfer.includes(item)), | ||
| ...items | ||
| .map(item => filterKey(item)) | ||
| .filter(item => keys.includes(item)) | ||
| ]); | ||
| // Get the items depending on if there is a search | ||
| const targetItemsToCheck = targetFilter | ||
| ? filteredTargetItems | ||
| : allTargetItems; | ||
|
|
||
| // Get the items keys | ||
| const targetItemKeys = targetItemsToCheck.map(item => filterKey(item)); | ||
|
|
||
| if (targetItemKeys.every(item => checked.includes(item))) { | ||
| // If all filtered items are checked, uncheck only those | ||
| setChecked(checked.filter(item => !targetItemKeys.includes(item))); | ||
| } else { | ||
| // Otherwise, check all filtered items | ||
| setChecked([ | ||
| ...checked, | ||
| ...targetItemKeys.filter(item => !checked.includes(item)) | ||
| ]); | ||
| } |
There was a problem hiding this comment.
The selection/deselection logic for both the source and target are the same (except the the filter and the targetItems. These could be passed as props to a common function.
There was a problem hiding this comment.
General question, why is the onChange callback exposed? Shouldn't we always use the same logic within the component?
| @@ -235,7 +237,7 @@ describe("TransferList", () => { | |||
| ); | |||
|
|
|||
| // test subheading is correct | |||
| expect(screen.getByText("2/3 selected")); | |||
| expect(screen.getByText("2 selected")); | |||
|
|
|||
There was a problem hiding this comment.
You could also sanity check here:
| // make sure target shows 0 selected | |
| const zeroSelectedHeadings = screen.getAllByText("0 selected"); | |
| expect(zeroSelectedHeadings.length).toBe(1); |
There was a problem hiding this comment.
I think there are a few test cases missing:
- When a user selects a value, searches for another value (list shows only one) selects this. Here we would expect to see the label to show
2 Selected(even though only one item is visible because of the search). In this state if we click the transfer button we would also expect both of the selected items to be transferred. - Say I have a the values [AAA, AAC, BBB]. I select BBB, now I search for "AA", I would expect to see ["AAA", "AAC"]. If I click the select all I expect to see
3 Selected. If I click the select all again I expect to see1 Selected. If I click select all again and clear the search I would expect to see3 Selected, if I click select all again I expect to see0 Selected - A test to make sure that the select all on the source list has no affect on the selection of the target list and vice versa.
|
Please remember to solve the merge conflict too |
Closes: TD-3359
Contributes to: TD-3333
Changes
Dependencies
N/A
UI/UX
Before:
After:
Testing notes
In the TransferList component:
Author checklist
Before I request a review:
I have tested the changes in Docker / a deploy-preview.I have populated the deploy-preview with relevant test data.I have tagged a UI/UX designer for review if this PR includes UI/UX changes.