determined
determined copied to clipboard
refactor: consolidate experiment list selection state
Description
This refactors some items related to the experiment list selection ahead of fixing the bulk action apis:
-
selection state (selected/excluded experiment ids, selectAll state) is consolidated into a single object. Previously, selected experiment ids, excluded experiment ids, and the selectall state were stored separately. This is problematic -- because the selected experiment ids were the source of truth for which experiments were selected, the stored list would grow as the user paged through the results when in selectall mode. we now determine the final list of selected elements on the fly.
-
grid selection is now fully determined from the id selection. previously, the grid selection and id selection were maintained separately. this required a lot of code ensuring they stayed in sync. we now only derive the grid selection from the selected id list.
-
handleSelectionChange now no longer requires row ranges for
add-all
orremove-all
change types. this allows for functions and effects that solely dealt with these change types to no longer rely on the experiments array. -
project experiment list settings is no longer using useSettings. because the grid selection now only updates when the settings updates, the act of updating the selection felt slower than when using useSettings. we now directly interface with the settings store at the project level -- global settings still use useSettings.
-
new hook: useTypedParams. In order to keep the functionality of the
compare
parameter, we introduce a useTypedParams hook which allows typesafe parsing and setting of query params similar to the settings store api. this should allow us to finally fully migrate off of useSettings.
Test Plan
Parameter/setttings behavior:
- navigate to a project experiment list with over 20 experiments in the list
- turn off infinite scroll and reload the page
- the experiment list should remain paginated
- click onto the second page
- the url should contain the query parameter
page=1
- reload the page
- the experiment list should load the second page
- click back onto the first page
- the url should no longer contain the page query parameter
- toggle the comparison view on
- the url should contain the query parameter
compare=true
- reload the page
- the experiment list should be in comparison mode
- remove the query parameter
compare=true
and reload the page - the experiment list should remain in comparison mode
- add the query paramter
compare=false
and reload the page - the experiment list should not be in comparison mode
Selection behavior:
- navigate to a project experiment list with over 20 experiments in the list
- select a single experiment in the list
- reload the page
- the experiment should remain selected
- navigate to the next page
- select a single experiment in the list
- navigate to the first page
- the selection status at the top of the table should show that two experiments are selected
- select an experiment, hold shift, and select another experiment
- all experiments between the first and second should be selected
- in the selection column menu, choose select all
- the selection status at the top of the table should show that all experiments are selected
- deselect an experiment by clicking a checkbox
- the selection status at the top of the table should respond likewise
Checklist
- [x] Changes have been manually QA'd
- [x] User-facing API changes need the "User-facing API Change" label.
- [x] Release notes should be added as a separate file under
docs/release-notes/
. See Release Note for details. - [x] Licenses should be included for new code which was copied and/or modified from any external code.
Ticket
this is base work for WEB-1937
Deploy Preview for determined-ui ready!
Name | Link |
---|---|
Latest commit | 5748bf4f244977e798028ad6c87e95535236c9df |
Latest deploy log | https://app.netlify.com/sites/determined-ui/deploys/65f35b0e04b2640008e861ed |
Deploy Preview | https://deploy-preview-8860--determined-ui.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Codecov Report
Attention: Patch coverage is 46.84096%
with 244 lines
in your changes are missing coverage. Please review.
Project coverage is 42.84%. Comparing base (
f73fd09
) to head (5748bf4
).
Additional details and impacted files
@@ Coverage Diff @@
## main #8860 +/- ##
==========================================
- Coverage 47.48% 42.84% -4.65%
==========================================
Files 1168 849 -319
Lines 176317 137164 -39153
Branches 2351 2385 +34
==========================================
- Hits 83729 58771 -24958
+ Misses 92430 78235 -14195
Partials 158 158
Flag | Coverage Δ | |
---|---|---|
harness | ? |
|
web | 42.96% <46.84%> (+0.13%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Files | Coverage Δ | |
---|---|---|
...t/src/pages/F_ExpList/F_ExperimentList.settings.ts | 100.00% <100.00%> (+55.97%) |
:arrow_up: |
webui/react/src/utils/asValueObject.ts | 95.30% <100.00%> (ø) |
|
webui/react/src/components/ExperimentMoveModal.tsx | 66.36% <50.00%> (ø) |
|
...act/src/pages/F_ExpList/glide-table/GlideTable.tsx | 16.19% <12.50%> (-0.04%) |
:arrow_down: |
webui/react/src/utils/observable.ts | 90.21% <41.66%> (-7.35%) |
:arrow_down: |
webui/react/src/hooks/useTypedParams.ts | 94.07% <94.07%> (ø) |
|
...c/pages/F_ExpList/glide-table/ColumnPickerMenu.tsx | 20.73% <27.77%> (-0.18%) |
:arrow_down: |
...src/pages/F_ExpList/glide-table/TableActionBar.tsx | 24.56% <20.00%> (-0.05%) |
:arrow_down: |
...bui/react/src/pages/F_ExpList/F_ExperimentList.tsx | 11.59% <8.13%> (+1.19%) |
:arrow_up: |
@EmilyBonar just pushed a fix that should address selections across pages and the compare param not persisting properly. This should also mitigate one source of the page being reset. could you confirm when you have the time?
The compare view and deselection across pages (bullet point 4) bugs are fixed. The bug with the page number getting reset on refresh is still present.
I'm still not entirely sure what the shift behavior is supposed to be, but when I follow this section of the test plan:
- the selection status at the top of the table should show that one experiment is selected
- hold shift and select an experiment
- the experiment should be added to the selection
The selection status shows that 2 experiments are selected, and the next experiment seems to be added to the selection whether or not shift is held.
updated the test plan -- i was thinking of the native glide table selection behavior, i've replaced it with how shift selections work currently. I've also pushed a potential fix for the page being reset by changing how we detect if the scroll took (previously we assumed that the table would be set up after a certain amount of renders with the glide table mounted, now we wait for the row in question to be in bounds) can you verify?
The page reset bug is fixed! The only step of the test plan I'm still having trouble with is
the selection status at the top of the table should show that one experiment is selected
When I reach that step I see "2 of [total] experiments selected"
@EmilyBonar updated the test plans again -- looking at the main branch that behavior is acceptable.