Remove hash update (and scroll-to bounce) from Diffs "Scroll To Next Unresolved..."
In !72278 (merged), I left this comment:
When I was initially working on the MR that led to me opening this issue in the first place, I noticed that after the initial scroll, the page would then scroll UP a little which I describe as a small "bounce" in the scroll-to behavior.
This MR still has that behavior (because it's intrinsic in the way we perform this action).
This presents a problem for the MR linked above, because we're marking a discussion as "seen" or "passed" when the user scrolls to it, and "unseen" or "next" when the user scrolls back up above the (arbitrary) line. Because of the bounce, every time the user clicks the jump button, it scrolls back up a little, resetting the "next" discussion to the one they just scrolled to. Obviously this is broken behavior.
There are a couple options to resolve this:
- Add some buffer to the intersection observer so that if there's some scroll up, it doesn't trigger
- Remove the scroll-up behavior entirely in the MVP so that it's unidirectional (e.g. once the user "sees" a discussion, they need to loop all the way back around before it'll ever be "next" again)
- Remove the bounce
@iamphill I'm going to open an issue to resolve this discussion, but I prefer option 3, and not just because it's by far the easiest option
😉
The issue I describe is caused by us both scrolling to a discussion (by pixel location) and then changing the hash in the browser to the file hash (which is used as the ID of the Diff File component).
Because a discussion on a file will necessarily be below the top of the Diff File, this always results in a scroll up.
We can resolve this in any of the ways I list above, but I want to expand on the third option (Remove the bounce), because I think it's the best option.
- We could invert the order, so that the hash is always first, and then the pixel value scroll happens. Because the top of the Diff File will always be above the discussion, this will always start a little higher, and then scroll a bit further to get to the discussion.
- We could remove the file hash update into the URL completely.
I think either option is good*, but I prefer number 2 for one reason: Updating the URL to point to the file hash seems a bit misleading. The user may wish to copy that link to send to someone ("check out this open discussion"), but a fresh load of the URL won't link to the discussion at all and - especially for large files - the discussion may not even be visible on screen.
This actually presents a third option:
- Update the hash to point to the
#note_[nn]
hash instead of the file hash.*
Do we think the file hash update in the URL is valuable when the user is jumping among discussions?
*: Assuming - of course - that I'm not missing some important detail about our rendering / scrolling, or a complication based in virtual scrolling.