Skip to content
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 ability for views to pin elements, such as headers and tracks #4237

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

garrettjstevens
Copy link
Collaborator

This adds the concept of pinning (or sticky positioning, using CSS positioning terminology) various UI elements.

First, the view menu bar is pinned for all views, so you can access things like the view menu even if you're scrolled far down in the view.

Views can then also use sticky positioning with any of their own elements they choose. In initial versions I said that you had to render the view component twice, once with sticky elements and once without, but that's changed now so that you only need to render it once. I did have to set the view menu height as a constant and export it from core so that views know where to pin themselves.

I then added pinned elements to the LGV. The header bar (and mini controls if the header bar is hidden) is now always pinned. Tracks can also be pinned from the "Pin track" entry in the track menu.

One thing to remember if adding pinning to future is that if you're using "position: sticky", make sure any non-sticky parent elements have "overflow: visible" set on them so that the sticky positioning ignores them (see MDN docs.

One possible addition is to add an indication that a track is pinned, like the shadow that appears under a pinned track in JBrowse 1.

Another possible addition, but likely in a separate PR, would be having the track labels be pinned as well.

Try it out here: https://s3.amazonaws.com/jbrowse.org/code/jb2/3685_pin_tracks/index.html?config=test_data%2Fvolvox%2Fconfig.json&session=share-n2-uSzxOMb&password=2Gb4e

Fixes #3685

@garrettjstevens garrettjstevens self-assigned this Feb 24, 2024
@cmdcolin
Copy link
Collaborator

this is really impressive. the usability is quite good in the demo link :)

haven't checked the code yet. the only thing that was notable during discussion was the division of the pinned and non-pinned tracks to incorporate a separate 'viewcontainer' (purple border) and it is sort of just a slight of hand that makes them look continuous.

while it looks pretty much fine in the UI, i would just want to do a little pondering whether there are any other implications of this

@cmdcolin
Copy link
Collaborator

one thing i saw, on embedded with the tracklist open, it is constantly re-calculating the width https://jbrowse.org/storybook/app/3685_pin_tracks/?path=/docs/getting-started--docs

video
Screencast from 2024-02-29 11-34-11.webm

@garrettjstevens
Copy link
Collaborator Author

the only thing that was notable during discussion was the division of the pinned and non-pinned tracks to incorporate a separate 'viewcontainer' (purple border) and it is sort of just a slight of hand that makes them look continuous

This actually got changed after the discussion, so it's a single view container in this PR.

one thing i saw, on embedded with the tracklist open, it is constantly re-calculating the width

Good catch, I'll look into it, I'm not sure off the top of my head why this change would cause that.

@garrettjstevens
Copy link
Collaborator Author

@cmdcolin I wasn't able to reproduce the behavior in the video you posted, either on Chrome or FireFox. The change I ended up making may still have helped, though, so I'd appreciate it if you could check on your computer after the latest changes have built.

I was able to fix the runaway dotplot resizing we discussed earlier. It was due to a change I made in the top-level app layout, I forgot to put in a max-width.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 72.41379% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 62.58%. Comparing base (a097560) to head (b739906).
Report is 39 commits behind head on main.

❗ Current head b739906 differs from pull request most recent head e7f2257. Consider uploading reports for the commit e7f2257 to get more accurate results

Files Patch % Lines
.../src/LinearGenomeView/components/VerticalGuide.tsx 0.00% 5 Missing ⚠️
packages/app-core/src/ui/App/ViewWrapper.tsx 72.72% 3 Missing ⚠️
...c/LinearGenomeView/components/LinearGenomeView.tsx 76.92% 2 Missing and 1 partial ⚠️
...ore/pluggableElementTypes/models/BaseTrackModel.ts 0.00% 2 Missing ⚠️
packages/app-core/src/ui/App/ViewContainer.tsx 85.71% 1 Missing ⚠️
...src/LinearGenomeView/components/RubberbandSpan.tsx 50.00% 1 Missing ⚠️
...iew/src/LinearGenomeView/components/TrackLabel.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4237      +/-   ##
==========================================
- Coverage   62.61%   62.58%   -0.04%     
==========================================
  Files        1088     1089       +1     
  Lines       31495    31529      +34     
  Branches     7531     7541      +10     
==========================================
+ Hits        19721    19731      +10     
- Misses      11602    11625      +23     
- Partials      172      173       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cmdcolin
Copy link
Collaborator

cmdcolin commented Mar 7, 2024

i believe the growth of the dotplot view still increases (until screen width) on embedded react app https://jbrowse.org/storybook/app/3685_pin_tracks/?path=/docs/getting-started--docs

random side note: it might be the use of a padding variable on the dotplot, which could potentially be removed

the calc 100vw - drawerWidth is 100 percent of the width of the whole viewport (screen) i believe so it still can increase without bounds

it may just be me also but i really like to avoid calc but if it works out, it might be ok

but i am curious because the grid, as weird as it is, has done well for us, are there any ways to get the system to work for the grid layout or can you explain the aspects that cause issues

random side note: the track selector opens very slowly after the dotplot view is open (not just at the importform), not exactly sure but it might be all related

@cmdcolin
Copy link
Collaborator

cmdcolin commented Mar 7, 2024

the padding diff

diff --git a/plugins/dotplot-view/src/DotplotView/components/DotplotView.tsx b/plugins/dotplot-view/s>
index 75389e705..e95ce7d43 100644
--- a/plugins/dotplot-view/src/DotplotView/components/DotplotView.tsx
+++ b/plugins/dotplot-view/src/DotplotView/components/DotplotView.tsx
@@ -28,7 +28,6 @@ const useStyles = makeStyles()(theme => ({

   container: {
     display: 'grid',
-    padding: 5,
     position: 'relative',
   },

@garrettjstevens
Copy link
Collaborator Author

Whatever was causing the problems I saw with the grid layout before must have been fixed by something else, so I've reverted back to the grid now.

I've also added a shadow below the pinned tracks, similar to JBrowse 1:

image

I feel like this is probably in a mergeable state, pending any more review comments.

@cmdcolin
Copy link
Collaborator

cmdcolin commented Mar 9, 2024

current branch maybe a horizontal overflow, allowing probably unwanted side scrolling

image

from https://jbrowse.org/code/jb2/3685_pin_tracks/?config=test_data%2Fvolvox%2Fconfig.json&session=share-DMFz_QXAmy&password=C1i8M

@cmdcolin
Copy link
Collaborator

cmdcolin commented Mar 9, 2024

nice work btw though :) it is coming along nicely

margin: theme.spacing(0.5),
padding: `0 ${theme.spacing(1)} ${theme.spacing(1)}`,
overflow: 'visible',
Copy link
Collaborator

@cmdcolin cmdcolin Mar 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these changes to overflow visible necessary in all cases? i tried removing these two for example and it appeared to "still work" so just curious. I find myself often wanting to "minimize the amount of css" so if there are code comments explaining if/why something is essential it would be good

}
}

const [pinnedTracks, unpinnedTracks] = partition(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personal pref/nit: i would use lineargenomeview/model.ts to make a getter for pinned/unpinned tracks with a simple filter like

+      /**
+       * #getter
+       */
+      get pinnedTracks() {
+        return self.tracks.filter(t => t.pinned)
+      },
+      /**
+       * #getter
+       */
+      get unpinnedTracks() {
+        return self.tracks.filter(t => !t.pinned)
+      },


that a) uses the mobx reactivity and b) might be a bit little simpler than custom partition function

@carolinebridge
Copy link
Contributor

Gonna +1 to this concept...while testing out #4252 noticing with "auto-adjusted" larger tracks, you'll definitely want the view menu to be accessible if you've got a long vertical space of tracks.

@cmdcolin
Copy link
Collaborator

there is some unexpected behavior with this PR with regard to breakpoint split view. I am not sure what to do about that, and it could be just a thing where we say it is sort of just something breakpoint split view users can be aware of. I am not sure if it's worth holding up this PR on its account

example:
scrolling the "whole page" can lead to the lines becoming unsynced

Screenshot 2024-04-15 at 10 47 19 AM

also rebased #4294 on this again, i think it should be a reasonable workaround

@garrettjstevens
Copy link
Collaborator Author

It turns out pinning tracks in a synteny view also doesn't work. It doesn't get unsynced like breakpoint split view, but the pinning just doesn't seem to work at all.

I think maybe we should disable pinning tracks on LGVs that are a child of another view for now, and then work on improvements after this PR is merged.

Also I took out the "overflow: visible"s since it turned out they weren't doing anything ("visible" is the default for overflow).

@cmdcolin
Copy link
Collaborator

I am not sure if this was fixed by that overflow:visible change. For example, if i browse the base branch you can visit this share link for example https://s3.amazonaws.com/jbrowse.org/code/jb2/3685_pin_tracks/index.html?config=test_data%2Fvolvox%2Fconfig.json&session=share-C6Qm2o4UOx&password=wDI4w and I can still side scroll the "View area" which is likely something we don't want. I cleared cache so i believe it is up to date

screenshot showing the side scroll

image

@garrettjstevens
Copy link
Collaborator Author

I think maybe we should disable pinning tracks on LGVs that are a child of another view for now, and then work on improvements after this PR is merged.

I've pushed a commit that does this.

@cmdcolin
Copy link
Collaborator

there are some minor glitches still with synteny view

for example if I scroll the 'outer page' on this session, the synteny canvas and the no track selected button both hover over the header

session https://s3.amazonaws.com/jbrowse.org/code/jb2/3685_pin_tracks/index.html?config=test_data%2Fvolvox%2Fconfig.json&session=share-CJe9lUKrGX&password=RB4kF

image

it may be hard to solve 100% of these issues

I guess part of me wonders if it would be good to have a setting to turn off the sticky header? not all users may like the sticky scrolling potentially.

@cmdcolin cmdcolin added the enhancement New feature or request label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pin track in JBrowse2
3 participants