fix: shift select #4636
fix: shift select #4636
Conversation
🦋 Changeset detectedLatest commit: f846dcb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4636 +/- ##
==========================================
+ Coverage 87.44% 87.48% +0.03%
==========================================
Files 335 339 +4
Lines 12733 12821 +88
Branches 3528 3542 +14
==========================================
+ Hits 11135 11217 +82
- Misses 1598 1604 +6
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| } | ||
|
|
||
| return () => { | ||
| mapCheckbox.delete(id) |
There was a problem hiding this comment.
we must remove the checkbox of the refList.current when the row is delete.
There was a problem hiding this comment.
it is done somewhere else (l.199 of ListContext.tsx)
| const { current } = checkboxRef | ||
|
|
||
| if (current) { | ||
| mapCheckbox.set(id, current) | ||
| if (refAtEffectStart && current && !refAtEffectStart.includes(current)) { |
There was a problem hiding this comment.
!refAtEffectStart.includes(current)
it's work ?
There was a problem hiding this comment.
it should be ok for me after considering @philibea feedbacks
fabienhebert
force-pushed
the
fix/shift-select
branch
from
010a4a5 to
cd66707
Compare
It was not intuitive & it is overridden by the new version of shift-select (similar to how it was before) |
lisalupi
force-pushed
the
fix/shift-select
branch
from
cd66707 to
6958b26
Compare
| useEffect(() => { | ||
| // Re-arrange refList everytime it is modified in order to keep the write order of the elements in it. | ||
| const allCheckboxes = [ | ||
| ...document.querySelectorAll('input[type="checkbox"]:not([value="all"])'), | ||
| ].map(element => element as HTMLInputElement) | ||
| refList.current = allCheckboxes.map(checkbox => checkbox) | ||
| }, [refList.current.length]) |
There was a problem hiding this comment.
By doing this you take the risk to select every checkbox in the page. Let's say you have a 2 list in the same page you will select checkboxes for both list while only one is being edited.
We should avoid using a querySelector for interacting with the list / table
| useEffect(() => { | ||
| const { current } = checkboxRowRef | ||
|
|
||
| if (current) { | ||
| mapCheckbox.set(id, current) | ||
| } | ||
|
|
||
| return () => { | ||
| mapCheckbox.delete(id) | ||
| } | ||
| }, [mapCheckbox, id]) |
There was a problem hiding this comment.
Why changing this? Here you automatically register and unregister the checkbox ref into the list provider and update the list of it depending on rows. This is useful especially if you have dynamic rows which happens quite often (deleting an element, etc)
Summary
Type
Summarise concisely:
What is expected?
Have a more intuitive behavior when shift+click on a selectable list/table (see 2. #4625)
The following changes where made:
Before:
Enregistrement.de.l.ecran.2025-01-03.a.16.40.49.mov
After :
Enregistrement.de.l.ecran.2025-01-03.a.16.41.16.mov