Make plots ribbon sticky on scroll #2759
Conversation
shcheklein
requested review from
mattseddon,
sroy3 and
julieg18
as code owners
shcheklein
force-pushed
the
stick-ribbon
branch
from
3522711
to
29d711e
Compare
shcheklein
added
product
shcheklein
force-pushed
the
stick-ribbon
branch
2 times, most recently
from
354f9cc
to
1062145
Compare
| @@ -1,4 +1,5 @@ | |||
| .webviewWrapper { | |||
| width: 100%; | |||
There was a problem hiding this comment.
Changing this to min-content would solve the following bug:
Screen.Recording.2022-11-14.at.8.58.31.AM.mov
There was a problem hiding this comment.
Technically, this bug is in main and not part of this pr. Could be fixed separately :)
There was a problem hiding this comment.
It is more apparent with a sticky banner though
There was a problem hiding this comment.
thanks @sroy3, good catch. The fix proposed changes the whole UX in a way that we probably don't want. Instead of the ribbon taking always size of the viewport only and overflowing items, it's now taking the whole space (defined by the most wide plot). I'm not sure this is a good idea. Also, plots size control will move to the edge if we do it this way
I can find a way to fill the space with something, but the whole layout might change after the #2747 and further changes?
There was a problem hiding this comment.
You are right, it is not as simple as that a change. What I'd do is make the ribbon 100% and create a div inside (ribbon-content or something else) and give it a width of 100vw (might have to calculate padding in there).
But yeah once #2585 is completely done, we shouldn't have horizontal scrollbars for the whole webview.
There was a problem hiding this comment.
let's skip it for now and get back to it after #2585 is done then
| top: 0; | ||
| width: 100%; | ||
| z-index: 100; | ||
| background-color: var(--vscode-editor-background); |
There was a problem hiding this comment.
variable $bg-color maps to var(--vscode-editor-background). We try to only use scss variables (easier to use and understand).
shcheklein
force-pushed
the
stick-ribbon
branch
from
36f914f
to
cfd8330
Compare
|
Code Climate has analyzed commit cfd8330 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 96.7% (0.0% change). View more on Code Climate. |
Closes #2731
Screen.Recording.2022-11-11.at.6.46.11.PM.mov
(writing it here primarily for the sake of future generation wondering how to use intersection observer API /
react-intersection-observerto handle top nav bar / menu that changes its size):rootMargincan be used to offset the root / parent element to the height of the menu. It works well if menu / nav bar height is stable. But would require creating a new observer when / if size changes. It breaks the point of using it and complicates code, etc, etc. See also Allow dynamically changing rootMargin w3c/IntersectionObserver#428thresholdis 1 or close to that)I was not able to make 1.0 threshold work (some pixels are not visible in the sticky mode?), but it work well with 0.95-0.99, etc.