Skip to content
This repository was archived by the owner on Jan 22, 2019. It is now read-only.

fix: sync z-index for github tree and hover overlay#278

Merged
ijsnow merged 2 commits intosourcegraph:masterfrom
lukeautry:luke/hover-overlay-zindex
Oct 31, 2018
Merged

fix: sync z-index for github tree and hover overlay#278
ijsnow merged 2 commits intosourcegraph:masterfrom
lukeautry:luke/hover-overlay-zindex

Conversation

@lukeautry
Copy link

@lukeautry lukeautry commented Oct 28, 2018

This PR changes:

Addresses an issue surfaced in https://github.com/sourcegraph/sourcegraph/issues/242 where a hover overlay gets covered up by the Github tree sidebar. This is a simple stack level bug.

Normally I would say to just update z-index for .hover-overlay but it would be really easy to reintroduce this bug with a simple edit to z-index on .tree-mount. I extracted out the value to a variables.scss file so that the z-index value be shared. I suspect that variables.scss is not a great place to put this and maybe there's an existing pattern to follow here when values need to be shared between scss files, but I couldn't find anything like that in this project.

Testing plan:

  • Shrink browser window so a tooltip is likely to overflow to the left
  • Hover over some symbol with information; should be easy to reproduce under the old code
  • Verify that overlay now goes over GitHub tree sidebar

I have tested on:

  • Chrome
  • Firefox
  • Safari
  • Phabricator Bundle

Copy link

@ijsnow ijsnow left a comment

Choose a reason for hiding this comment

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

Hello and thanks for looking into this! Just one question below.

top: 0;
left: 0;
z-index: 2000;
z-index: $default-z-index;
Copy link

Choose a reason for hiding this comment

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

From what I can tell, making the z-index be the same for both the hover overlay and the tree mount only fixes the problem because the hover overlay's mount is appended to the end of document.body but the tree mount is added further up the DOM tree? It's important to note that it doesn't actually need to be. It could just be added to the bottom making this fix not really deterministic.

What about subtracting one for the default for this? Or just making them 2000 and 1999?

Suggested change
z-index: $default-z-index;
z-index: $default-z-index - 1;

Copy link
Author

Choose a reason for hiding this comment

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

This is a good point in that restructuring the order of how this stuff gets injected could negate these changes. Pushing up the change.

@ijsnow
Copy link

ijsnow commented Oct 31, 2018

Great, thanks!

@ijsnow ijsnow merged commit f195537 into sourcegraph:master Oct 31, 2018
@sourcegraph-bot
Copy link

🎉 This PR is included in version 1.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants