Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Allow changing the overlay mount location #8340

Merged
merged 2 commits into from
Feb 10, 2020
Merged

Conversation

lguychard
Copy link
Contributor

Rel https://github.com/sourcegraph/sourcegraph/issues/3228#issuecomment-481174882

On GitLab merge request pages, the diff tab is always present. As a result, when switching tabs, the hover overlay remains visible, since it is by default mounted to document.body, and as such is not affected by the change in visibility of the diff tab.

This PR introduces the ability to customize the hover overlay mount location, through CodeHost.getHoverOverlayMountLocation(). If CodeHost.getHoverOverlayMountLocation() is unspecified, or if it returns null, the hover overlay will be mounted to document.body. If the custom mount location is removed from the DOM, the hover overlay will be mounted to document.body again.

@lguychard lguychard requested review from felixfbecker and a team February 10, 2020 10:28
Rel https://github.com/sourcegraph/sourcegraph/issues/3228#issuecomment-481174882

On GitLab merge request pages, the diff tab is always present. As a result, when switching tabs, the hover overlay remains visible, since it is by default mounted to `document.body`, and as such is not affected by the change in visibility of the diff tab.

This PR introduces the ability to customize the hover overlay mount location, through `CodeHost.getHoverOverlayMountLocation()`. If `CodeHost.getHoverOverlayMountLocation()` is unspecified, or if it returns `null`, the hover overlay will be mounted to document.body. If the custom mount location is removed from the DOM, the hover overlay will be mounted to document.body again.
@lguychard lguychard force-pushed the lg/fix-gitlab-hover-mr branch from 5aa7f93 to ee2c93f Compare February 10, 2020 10:36
@felixfbecker
Copy link
Contributor

Code LGTM. For my understanding, could you describe what happens (in this code) specifically in Gitlab when the user is switching tabs on an MR?

@lguychard
Copy link
Contributor Author

lguychard commented Feb 10, 2020

  1. User visits https://gitlab.com/gitlab-org/gitaly/-/merge_requests/1575. div.tab-pane.diffs doesn't exist yet (it'll be lazy-loaded) -> the hover overlay is mounted to document.body.
  2. User visits the 'Changes' tab -> the hover overlay is unmounted from document.body, mounted to div.tab-pane.diffs
  3. User visits the 'Overview' tab again -> div.tab-pane.diffs is hidden, and as a result so is the hover overlay.

Then there's some defensive code accounting for div.tab-pane.diffs being removed (for example during a navigation away from the MR that only results in a soft reload), to remount the hover overlay to document.body. I haven't encountered this case in practice, but thought it best to account for it.

@lguychard lguychard mentioned this pull request Feb 10, 2020
37 tasks
@felixfbecker
Copy link
Contributor

That could be great to add in a comment somewhere as an example!

@lguychard
Copy link
Contributor Author

Added as a doc block to observeHoverOverlayMountLocation().

@lguychard lguychard force-pushed the lg/fix-gitlab-hover-mr branch from 0fb0f8d to 9f1b1fc Compare February 10, 2020 12:27
@lguychard lguychard merged commit 85855c4 into master Feb 10, 2020
@lguychard lguychard deleted the lg/fix-gitlab-hover-mr branch February 10, 2020 12:33
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants