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

Fixing - screenshot preview is overlaying permalink #2187

Closed
wants to merge 7 commits into from

Conversation

@rbrishabh
Copy link
Contributor

rbrishabh commented Aug 1, 2019

Fixes #1274

Hi @julienw and @canaltinova !
I hope you guys had wonderful holidays :)

I am creating a pull request because this way it is easier for you guys to see what changes need to be done -- as suggested by @julienw .

How does this look?
Does this need any more changes?

Thanks!

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #2187 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2187      +/-   ##
==========================================
+ Coverage   85.75%   85.75%   +<.01%     
==========================================
  Files         200      200              
  Lines       13919    13925       +6     
  Branches     3503     3506       +3     
==========================================
+ Hits        11936    11942       +6     
  Misses       1817     1817              
  Partials      166      166
Impacted Files Coverage Δ
src/components/timeline/TrackScreenshots.js 94.25% <ø> (ø) ⬆️
src/test/fixtures/utils.js 97.87% <100%> (+0.31%) ⬆️

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 7161bf5...374b84b. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #2187 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2187      +/-   ##
==========================================
- Coverage   85.83%   85.75%   -0.08%     
==========================================
  Files         200      200              
  Lines       13985    13925      -60     
  Branches     3529     3503      -26     
==========================================
- Hits        12004    11942      -62     
- Misses       1815     1817       +2     
  Partials      166      166
Impacted Files Coverage Δ
src/components/timeline/TrackScreenshots.js 94.25% <ø> (ø) ⬆️
src/test/fixtures/utils.js 97.87% <100%> (+0.31%) ⬆️
src/components/sidebar/CallTreeSidebar.js 87.67% <0%> (-6%) ⬇️
src/components/shared/StackSettings.js 85.71% <0%> (-4.77%) ⬇️
src/profile-logic/call-tree.js 97.48% <0%> (-0.63%) ⬇️
src/profile-logic/transforms.js 90.25% <0%> (-0.45%) ⬇️
src/test/fixtures/profiles/processed-profile.js 97.06% <0%> (-0.19%) ⬇️
src/selectors/per-thread/thread.js 97.91% <0%> (-0.09%) ⬇️
src/profile-logic/profile-data.js 93.43% <0%> (-0.02%) ⬇️
src/profile-logic/comparison.js 92.25% <0%> (ø) ⬆️
... and 5 more

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 f9bf244...d3088f1. Read the comment docs.

rbrishabh added 2 commits Aug 1, 2019
res/css/style.css Outdated Show resolved Hide resolved
#root-screenshot {
z-index: 1;
}

This comment has been minimized.

Copy link
@julienw

julienw Aug 2, 2019

Contributor

So, I looked at the result of this PR, and it doesn't change anything to the behavior.
The reason is simple: we want the #root to have a bigger z-index that the screenshots.
So I think this one should go to 0, and the one above to 1.

The result should be that the screenshots go below the other overlays, but above the rest of the chrome.

Note: this is the current behavior:
Capture d'écran de 2019-08-02 14-34-45

i hope this is clearer!

This comment has been minimized.

Copy link
@rbrishabh

rbrishabh Aug 2, 2019

Author Contributor

So, I looked at the result of this PR, and it doesn't change anything to the behavior.
The reason is simple: we want the #root to have a bigger z-index that the screenshots.
So I think this one should go to 0, and the one above to 1.

The result should be that the screenshots go below the other overlays, but above the rest of the chrome.

Alright, got it! Thanks for the great explanation!

i hope this is clearer!

Definitely clearer!

I actually scratched my head around on how to test the changes I am doing, but I couldn't figure it out! Thanks for your help! Could you still please test me how to check the changes I am making for this one?

That way, next time - I can submit PRs in a much smarter way haha. Thanks =)

Copy link
Contributor

julienw left a comment

Thanks for the changes, but I realized that the fix should be backwards. See below for more explanation!

rbrishabh added 2 commits Aug 2, 2019
@julienw

This comment has been minimized.

Copy link
Contributor

julienw commented Aug 2, 2019

Don't worry about tests hard, we'd need visual tests to test css changes and we don't have this. The changes you added in existing tests check properly the change in the root overlay, so we're good :)

@rbrishabh

This comment has been minimized.

Copy link
Contributor Author

rbrishabh commented Aug 2, 2019

Alright @julienw !

So this is ready to be merged? =)

@julienw

This comment has been minimized.

Copy link
Contributor

julienw commented Aug 2, 2019

@rbrishabh

This comment has been minimized.

Copy link
Contributor Author

rbrishabh commented Aug 3, 2019

Sorry, I couldn't try the new patch before leaving work today, and I'll make sure to check it out Monday! Thanks again for your work :)

No problem at all man!
You're welcome :)
Also, if I can be of service anywhere else, I would love to do so, so you can always tag me =)

@julienw

This comment has been minimized.

Copy link
Contributor

julienw commented Aug 5, 2019

So, now no screenshot work at all, neither do tooltips, probably because #root wasn't the thing to change.
I think this needs some investigation that I don't have time to do now. I'd be glad if you can do it, but otherwise we can close this PR and remove the good first issue label on the issue, because that isn't a good first issue. Sorry about that, and thanks for looking at it already!

@rbrishabh

This comment has been minimized.

Copy link
Contributor Author

rbrishabh commented Aug 5, 2019

I would definitely like to look into it.
Could you please just tell me how can I reproduce this problem locally?
What do I need to do to do that?

I tried to do it earlier, but wasn't successful. Thanks!

@julienw

This comment has been minimized.

Copy link
Contributor

julienw commented Aug 5, 2019

Here is how you can reproduce this:

  1. Open this profile: https://perfht.ml/2MKT5Hy
  2. Open one of the panels at the top (eg permalink, or publish)
  3. hover the screenshots below
    => as you'll notice, the screenshots are going over the panels. But they should go below, ie, the panels should be on top.

The solution will involve z-index, but also maybe more complex changes. I don't know before looking and I don't have time now, sorry :(

@rbrishabh

This comment has been minimized.

Copy link
Contributor Author

rbrishabh commented Aug 5, 2019

Ahh I got it. I would love to look into this a bit more.

The solution will involve z-index, but also maybe more complex changes. I don't know before looking and I don't have time now, sorry :(

No worries, if I do find anything, I will comment here, if not, I will close this PR.

@rbrishabh

This comment has been minimized.

Copy link
Contributor Author

rbrishabh commented Aug 21, 2019

I did try to fix this by changing a lot of values! I couldn't find a solution! I am closing this for now! I will reopen it if I do find something! =)

@rbrishabh rbrishabh closed this Aug 21, 2019
@julienw

This comment has been minimized.

Copy link
Contributor

julienw commented Aug 22, 2019

Thanks for trying, at least now we know the problem is more complex than we thought!

@rbrishabh

This comment has been minimized.

Copy link
Contributor Author

rbrishabh commented Aug 22, 2019

Yep @julienw! Definitely worth the try!

A lot to learn here.

Meanwhile, if I can help with any other issue, I would love to do that! Just tag me =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.