Make plots vertical scrollbars local to sections #3489
Conversation
Testing for this kind of thing inside jsdom is near impossible and does not tell muchunfortunately. |
There was a problem hiding this comment.
Should the pinned plot have a background color?
Screen.Recording.2023-03-17.at.3.44.10.PM.mov
There was a problem hiding this comment.
Actually, it looks the same on main. Hopefully, it's expected and not a bug.
There was a problem hiding this comment.
Can we either fix in a follow up or raise a bug please.
There was a problem hiding this comment.
Will fix straight away as a follow-up
| const headRef = createRef<HTMLTableSectionElement>() | ||
|
|
||
| useEffect(() => { | ||
| const calculateTop = () => { |
There was a problem hiding this comment.
Maybe it's just on my end (or I just scroll more aggressively 😄), but the image headers look really buggy...
Screen.Recording.2023-03-17.at.3.48.38.PM.mov
Should we consider not making the image headers sticky at all?
There was a problem hiding this comment.
We do need sticky headers for the revisions. I am also seeing some jank:
Screen.Recording.2023-03-20.at.9.37.33.am.mov
Note: There is a visible gap between the normal sticky headers and the ribbon in the video.
There was a problem hiding this comment.
Maybe it's just on my end (or I just scroll more aggressively 😄), but the image headers look really buggy...
Should we consider not making the image headers sticky at all?
Went back and tried making this faster. Decided to avoid React as much as possible for the style update. useState is a little too efficient with reconciliation and sometimes makes things a little less smooth. It's is not as perfect as it would with CSS only, but it's way much better now.
Screen.Recording.2023-03-20.at.10.07.29.AM.mov
There was a problem hiding this comment.
We do need sticky headers for the revisions. I am also seeing some jank:
Note: There is a visible gap between the normal sticky headers and the ribbon in the video.
I am not able to reproduce. Might have to do with resolution and unfortunately I cannot test on a bigger screen at the moment. Can you check PlotsContainer.tsx on line 138 (stickyHeaderTop={ribbonHeight - 4}) and play with a higher than 4 value and tell me which value would close that gap for you? Removing a few pixels from the header won't be that apparent for other users, but if it closes the gap for users like you that'd would make it perfect.
mattseddon
left a comment
mattseddon
left a comment
There was a problem hiding this comment.
Happy to merge and follow up
| const headRef = createRef<HTMLTableSectionElement>() | ||
|
|
||
| useEffect(() => { | ||
| const calculateTop = () => { |
There was a problem hiding this comment.
We do need sticky headers for the revisions. I am also seeing some jank:
Screen.Recording.2023-03-20.at.9.37.33.am.mov
Note: There is a visible gap between the normal sticky headers and the ribbon in the video.
|
Code Climate has analyzed commit 7cafa31 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.0% (0.0% change). View more on Code Climate. |
3 participants
Part of #2585
Screen.Recording.2023-03-17.at.11.17.41.AM.mov
Comparison table headers while scrolling isn't as smooth as it used to due to the fact that we can't have sticky positioning inside of a container that has an overflow. Instead of using sticky positioning in CSS, the feature was recreated in Javascript.
Do we want local scrollbars inside of template plots as well? For the multi view plots specifically.