Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix screenshot stacking with respect to permalink overlay #3073

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Riju19
Copy link
Contributor

@Riju19 Riju19 commented Nov 21, 2020

This pull request changes the way screenshot-hover is rendered so that it has same parent stacking context as permalink, and therefore enables the stacking to be ordered by their z-index.
Fixes #1274

┆Issue is synchronized with this Jira Task

@codecov
Copy link

codecov bot commented Nov 21, 2020

Codecov Report

Merging #3073 (25e549a) into main (144758c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3073   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files         240      240           
  Lines       19100    19106    +6     
  Branches     4880     4885    +5     
=======================================
+ Hits        16833    16839    +6     
  Misses       2100     2100           
  Partials      167      167           
Impacted Files Coverage Δ
src/components/app/ProfileViewer.js 84.61% <ø> (ø)
src/components/timeline/TrackScreenshots.js 94.84% <100.00%> (ø)
src/test/fixtures/utils.js 95.65% <100.00%> (+0.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 144758c...25e549a. Read the comment docs.

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks good!

I noticed we have another problem around context menus:
image

Can you please look if increasing the z-index in ContextMenu.css (currently it's 4) will fix this? Also if this work, please change the comment there adding a mention about screenshots. If this doesn't work please ignore this and we can file another issue about that. (but I'm confident this will work).

Thanks again for your patience!

(Note: this is the profile I've been using here: full link)

z-index: 10;
z-index: 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why this change? I mean, I'm not opposed :-) but this doesn't seem useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was just meant to put z-indexes in order. The element below this one has an index of 4, so I just ordered them that way, so that we don’t use arbitrary numbers.
However, I’m not sure if it was intentionally given a large number. If so, we can definitely change it back to 10 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think this is good, more "ordered". Especially if we sue "10" this should be explained better than just an arbitrary number, so in this case "5" is just as good. Thanks :-)

Projects that are a bit more in order than our project for z-index do have a clear plan about them. They also keep some "space" so that it's possible to add elements between the stacks without changing everything. We're not there yet!

@julienw julienw self-requested a review April 26, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screenshot preview is overlaying permalink
2 participants