Adds the ability to highlight regions using the bookmarks widget #4003
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4003 +/- ##
==========================================
- Coverage 63.24% 63.16% -0.08%
==========================================
Files 1045 1045
Lines 30616 30695 +79
Branches 7314 7329 +15
==========================================
+ Hits 19362 19388 +26
- Misses 11084 11138 +54
+ Partials 170 169 -1 ☔ View full report in Codecov by Sentry. |
|
Idea from lincoln: 
might help navigating bookmarks with labels ... icon would be toggle-able on/off thoughts? |
|
yep labels, even displayed textual labels (e.g. for screenshots) but the mouseover is nice too, can be quite useful |
|
reminds me that the highlight may want to be something that can be svg exported too |
…dds new dialog for bulk editing highlight color and session wide options for showing/hiding highlights
|
From meeting:
|
carolinebridge
changed the title
| } | ||
|
|
||
| let bookmarkWidget = session.widgets.get('GridBookmark') as GridBookmarkModel | ||
| if (!bookmarkWidget) { |
There was a problem hiding this comment.
I think it might be worth avoiding doing 'side effects' like addWidget in the body of the react render (e.g. move to useEffect if needed? if its needed at all?)
There was a problem hiding this comment.
It's needed to access the bookmarks without retrieving directly from the localstorage (i.e. it's going to be using the existing grabs within the bookmarks model instead of re-writing them all)
the motivation here was basically, if you're viewing highlights you're probably going to want to be using the bookmarks widget anyways
i moved it into a useState ? not sure if this resolves your concern but lemme know if you have a better soln for this guy
There was a problem hiding this comment.
reverted cus the linter reminded me why i did it like this in the first place:
React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render thoughts?
There was a problem hiding this comment.
move to a useEffect instead of a useState. useEffect is a good way of expressing that "you are doing something as a sideneffect of rendering" which in this case, as part of rendering this component, you are opening the bookmark widget.
There was a problem hiding this comment.
also, the linter/runtime warning there is correct: gotta follow the rules of hooks https://legacy.reactjs.org/docs/hooks-rules.html (or new docs https://react.dev/warnings/invalid-hook-call-warning)
There was a problem hiding this comment.
sorry, what i meant was using a useEffect has the same linter rule invoked, not that it was exclusive to useState or that the linter was wrong for calling it out. I think it's because I return early if isSessionModelWithWidgets is false
There was a problem hiding this comment.
@cmdcolin lemme know how that looks in the most recent commit
plugins/grid-bookmark/src/GridBookmarkWidget/components/Highlight/Highlight.tsx
Show resolved
Hide resolved
plugins/grid-bookmark/src/GridBookmarkWidget/components/BookmarkGrid.tsx
Outdated
Show resolved
Hide resolved
| self.bookmarks.remove(bookmark) | ||
| } | ||
| } | ||
| self.setBookmarkedRegions( |
There was a problem hiding this comment.
any reason for this change?
There was a problem hiding this comment.
iterating through the array of bookmarks and then removing them was causing issues when clearing all bookmarks -- not all bookmarks were cleared. this function is only used when you attempt to delete bookmarks with nothing selected ('clear' operation)
filtering out bookmarks not present in the session's assemblies then casting the property in setBookmarkedRegions seems to work better.
There was a problem hiding this comment.
that is a good bugfix, but it calls into question what was wrong with the previous approach. does the observable-array::remove function not work properly here, or is something not properly observing it
There was a problem hiding this comment.
(i can reproduce the issue where 'delete'/clear all was not working before)
plugins/grid-bookmark/src/GridBookmarkWidget/components/Highlight/Highlight.tsx
Outdated
Show resolved
Hide resolved
| className={classes.highlight} | ||
| style={{ left, width, background: highlight }} | ||
| > | ||
| {showBookmarkLabels ? ( |
There was a problem hiding this comment.
you could consider drawing the text right onto the display. this would make it easier to see at a glance, and allow screenshot-ability compared to requiring a mouseover
There was a problem hiding this comment.
do you mean have the label displayed at all times? is that desirable?
There was a problem hiding this comment.
IMO, yes. the bookmark icon feels odd to me, especially if there is not any label. i think it may look better for there to be no icon, and text displayed always, unless showBookmarkLabels is false
There was a problem hiding this comment.
Not sure I agree with this --
You can turn the bookmark icon 'off' for the express reason of "I don't want this in my screenshots". Being enabled by default communicates to the user "what" the highlighted area is, and enables the label quick access. The icon, for this purpose, was requested by Lincoln when I first demo'd it to him (it looked like the first screenshot on this PR).
If a user really wants their labels in their screenshots, delayed screen captures exist. I think having so much text like that, especially for a heavy "annotator", would be obtuse.
Mostly though, I really don't see having the text displayed always as a better-looking option...I am having trouble conceiving what you might be imagining, here's a mockup?:
Maybe draft for me what you'd like to see or describe some more details.
There was a problem hiding this comment.
there are similar challenges with overlapping feature labels. I believe such overlapping text could be remedied in most cases by adding ellipses and/or perhaps adding y-axis dodging (so that they avoid being on the same text row)
i actually prefer the text being there despite overlaps, because it gives me a quick overview, without having to mouseover many things. it is up to you though
There was a problem hiding this comment.
I'll leave it as is for now...I think resolving the overlaps with an ellipses negates the idea of having an overview of the labels (because you can't read them in their entirety), and if you're looking for a big picture overview, you can see all your labels in the widget. Y-axis dodging could resolve for some cases, but I don't think it would scale well, because in higher density you're adding a lot of vertical space clutter.
| 'GridBookmarkWidget', | ||
| 'GridBookmark', | ||
| ) as GridBookmarkModel | ||
| bookmarks.current = newBookmarkWidget.bookmarks |
There was a problem hiding this comment.
i think that this code points to a thing where the fact that the bookmarks are stored on the widget, and therefore needing the widget to be open, is a bit awkward.
one alternative approach would be to extend the session model (via Core-extendSession perhaps) and add the functions that read and manipulate the highlight state directly onto the session model. then, the highlights can be displayed regardless of whether the grid bookmark widget is opened.
i would say this change is not necessarily required for this PR to go forward but it could be worth considering for the future
There was a problem hiding this comment.
(note that one reason i say it's not strictly required is that the bookmark widget does not strictly need to even be "open" as in visible, as we have the concept of the widget state being retained in the session despite it being "closed", at least for now...xref #3538)
|
this is sort of a misc proposal, but I tried out a proposal for moving basically all the buttons in the 'header' of the bookmark widget into a dropdown hamburger menu example screenshot
it also adds a edit icon to all the label fields, to try to make it more abundantly clear that the labels are editable, and then we don't need the 'alert' box in the header. it removes the 'editable:true' on the label column because hitting enter triggers the dialog, but we could re-enable the editable:true if it didn't trigger the dialog on enter perhaps, but now this just makes editing always needs the dialog |
|
the code diff with whitespace removed is here https://github.com/GMOD/jbrowse-components/compare/bookmark-highlights...bookmark-highlights-cmd2?expand=1&w=1 |
| @@ -175,6 +221,16 @@ export default function f(_pluginManager: PluginManager) { | |||
| .actions(self => ({ | |||
| afterAttach() { | |||
| const key = localStorageKeyF() | |||
| function handler(e: StorageEvent) { | |||
There was a problem hiding this comment.
tested and works :) I think this should be good to go
|
re: bookmarks-highlights-cmd2 I don't mind the hamburger menu, but I think the edit icon doesn't "fit" in the table cells. Longer labels seem to cut it off on the default load. Might be better to put it in the column header, if anywhere. I don't mind removing the single click edit if it felt unnatural. |
|
what if assembly column was hidden by default? the majority use case would be showing 'one genome assembly's worth of bookmarks at a time' and this would provide more room for edit icon |
|
I mostly think the edit icon looks out of place in the cell, here's what the table looks like at the default size with the assembly column hidden: 
I don't mind hiding that column when one assembly is shown, but I'm not sure that solves the awkwardness. |
|
with the "classes.cell" css class is added to this div, it will more accurately change the cell to have ellipses instead of cutting off the icon like in the above screenshot. added that as a push to branch |
| const [widths, setWidths] = useState([ | ||
| 50, | ||
| 40, |
There was a problem hiding this comment.
I think that the checkbox cell is still width ~50 so this change may not be accurate. can see in hovering the lines are mismatched
|
i'd be fine with removing the icon for now also...maybe can just do that and merge the hamburger menu in, and then ponder other edit icon stuff later |
cmdcolin
force-pushed
the
bookmark-highlights
branch
from
19b7990 to
219b45a
Compare
|
i think good to go, possible add on here #4130 |
|
I had to move some of the 'unit tests' into integration level tests to allow the new usage of session.queueDialog from the hamburger menu. This is because session.queueDialog will not render the dialog properly in unit tests, because the app-level processing that actually react-renders the dialog queue is not hooked up |
demo: https://s3.amazonaws.com/jbrowse.org/code/jb2/bookmark-highlights/index.html
I think this idea could address #599
leverages code from #3648
includes fix for #3968 that can be cherry picked out if desired