-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add sorting and collapsing options to the hierarchical track selector #3810
Conversation
c157b42
to
6837217
Compare
Codecov Report
@@ Coverage Diff @@
## main #3810 +/- ##
==========================================
- Coverage 64.42% 64.34% -0.09%
==========================================
Files 995 998 +3
Lines 29563 29648 +85
Branches 7092 7114 +22
==========================================
+ Hits 19047 19078 +31
- Misses 10355 10409 +54
Partials 161 161
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
initial comment said I didn't add "collapsing via config" but now there is: configuration.hierarchical.defaultCollapsed.categoryNames:stringArray configuration.hierarchical.defaultCollapsed.topLevelCategories:boolean configuration.hierarchical.defaultCollapsed.subCategories:boolean These are only done on initial load of the session, after that the collapsed state is maintained in the session of the collapse takes over (note: the storage of collapse state in session could get large/bloat session potentially but that "issue" has always existed) |
requested review @carolinebridge-oicr @garrettjstevens just for clarity of UI and perhaps also the config concepts. this is one of the more "nested" configs out there but I think it helps organization and clarity. the alternative is lifting the configs and including their nested purpose in their name e.g. configuration.hierarchical.sortTopLevelCategories=true |
I think the nesting of the configs is fine, as long as it's documented it should be fine. One question I did have was, is there a way to set the hierarchical options in the config when using the admin server? I seems since it's all loaded in the session after initial load that any changes an admin makes using the admin server wouldn't get saved to the config. |
}, | ||
{ type: 'divider' }, | ||
{ | ||
label: 'Collapse subcategories', |
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 think this should be a toggle to expand/collapse all subcategories
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.
by toggle do you mean checkbox? i avoided checkbox because you can get into states that are not 'all or nothing', e.g. you could expand all, then collapse one manually, so then it's not in an expand all state
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.
It doesn't have to be a checkbox, you can just ternary on a boolean for the text / icon / behaviour --
For expanding / collapsing I would assume the behaviour there would be:
Collapse all (all categories are open) -- lets the user see all their category headers and select the one relevant to them from there
Expand all (some or no categories are open) -- lets the user explore their tracks individually if they had collapsed them all.
Maybe this is just me though for expected behaviour. The first thing I did after clicking "collapse categories" was click it again expecting them to expand.
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.
it is somewhat tempting to make it a toggle, but i kind of prefer the current behavior because it makes it clear that it's an acton that manipulates the state, and that you can repeat it if you want, rather than being a true toggle. i tried to in the below comment remove the subcategories option when no subcategories are available, to reduce cognitive load
onClick: () => model.collapseTopLevelCategories(), | ||
}, | ||
{ | ||
label: 'Expand all categories', |
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.
Overall I don't think there's enough distinction between what these options do...
Collapse top-level categories does the same thing that subcategories does on the demo link?
They should all be toggles to expand all/collapse all
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.
They are slightly different behaviors, see the behavior on the volvox example. possibly the subcategories one could be hidden if no subcategories are even available on track selector
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 now removed the subcategories option if there are none, which may remove some cognitive overload from the unnecessary option
setMenuEl(undefined) | ||
}} | ||
onClose={() => setMenuEl(undefined)} | ||
<CascadingMenuButton | ||
menuItems={[ |
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.
Add track is no longer at the top here -- subjective, but I think some users might get lost when looking for this option. Maybe we should consider restricting this hamburger menu to just be manipulating the listed options i.e. expand/collapse/sort, and have separate button / ui for manipulating tracks or connections.
One option would be to make the Add track FAB have the text "Add track" with the plus Icon and then allow the user to select adding a track or connection...could add a connections manager, etc.
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.
the discoverability of "Add track" is a persistent problem. I'd be happy to sort it back to the top but ya, the current 'state' of the system is that add track options are in multiple places (Fab, Hamburger menu, top level menu) in the UI because of this discoverability problem
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 was messing around with connections as well while exploring this and the UI for it is suuuuper clunky with the multi modals.
I don't believe we have a "config editor" for the global jbrowse.configuration field, but for the collapse options in particular, changing those options in the yet-to-be-created-global-config-editor would not change the result because the "my take on it" at least is that the collapse options are only applied on session load, which sets a "initialized" field on the hierarchical widget model. it could be the initialized thing gets re-toggled off if the config value is detected to change though. |
sorted these options to the bottom of hamburger menu now |
4a2658d
to
525000e
Compare
Fixes #26
This PR offers new options via config to sort the track selector via top level configuration.hierarchical.sort.trackNames and configuration.hierarchical.sort.categories
Screenshot of hamburger menu open with new sort+collapse options
"Collapse subcategories"
"Collapse top-level categories"
"Sort categories by name"
can see the above has category names "Alignments","Bigwigs", etc. sorted alphabetically
"Sort tracks by name"
Can see that categories are not sorted (Miscellaneous comes before Alignments category) but track names are sorted