issue: better better fix for #598

[the-j0k3r]

REALLY Does fix both issues described in #598 for single and double height file-headers

Its not perfect but works as opposed to current master
This code came from @silverwinds snippets in #598, I polished it and heavily tested it in Firefox, Opera and Google Chrome against the issues it fixes and introducing any newer issues.

See discussion in 5a246df#comments as indicated below.

Pros:

• Doesn't transform single height file-info file-header height. (better than current master)
• Line selection is visible no matter file-header height single or double height. (better than current master)
• Selected line remains selected and visible even after page refreshes.(better than current master)
• Selected line remains selected and visible even after browser restarts. (better than current master)
• Works even if user chooses bigger font in Google Chrome, Opera and if user has a minimum font size set in Firefox. (better than current master)
• Fixes gap discussed in Broken scroll element behaivior #612 (better than current master)
• Reduced gap at top between pr-toolbar and file-info header see Floating header in PR files view covers selected line #598 (comment) (better than current master)
• Neither of the cons are a deal breaker for me.

Cons:

• On Double height file-headers, clicking twice on same line pushes the selection up its still visible but ideally shouldn't do this.
• The selected line is a little too far down past the file-header even the double height.
• When opening a selected line direct links in new tab the line is still under headers, (same as current master)
• Slightly bigger gap at top between pr-toolbar and file-info header see Floating header in PR files view covers selected line #598 (comment).
• It has lots of values in rule that are not colors but are necessary to solution @Mottie probably wants a detailed justification for each. </sarcastic grin here>

[the-j0k3r]

@maxrothman @auscompgeek

Would you guys mind testing current master vs this PR against bug #598 issue 1 and issue 2, please?

Test sites with single and double height header (minimum font size set to 14px in Firefox and Google Chrome) https://github.com/wpilibsuite/allwpilib/pull/1022/files#diff-b0bc4c1c897b5abdb841f75a8f7896f7R3 and other sites you may use incidentally

Thanks.

[Mottie]

If you want my review, I'd say...

1. The heck with this trouble, lets rip it out by the roots and move the css into another repo, since it's opinionated.
2. I totally can't reproduce this in Chrome, so my opinion on this is sure, go ahead and add it. If it ever breaks, or someone has an issue with it, refer to 1.

[the-j0k3r]

I vote for that, sort of one last try.

[xt0rted]

It's like 5 lines in a new style (if you don't care about the selected line being hidden). Seems better to move this out than to add a ton of hacks in for something that's not part of GitHub right now and can't be properly fixed with css alone.

I was also thinking that the stuff for Refined GitHub could be moved into a Refined-GitHub-Dark repo or something.

[the-j0k3r]

It's like 5 lines in a new style (if you don't care about the selected line being hidden)

I do personally mind,but in an earlier conversation with @Mottie over at discord I did suggest this be moved to its own style given the fuss.

Theres two options on that front, since this is sticky header stuff, theres already one style for the main header, with a little work the usercss style could have some dropdown settings for each sticky portion like enable yes no sort of thing so it can suit a wider audience and you can decide what to stick.

Or make yet another repo for refined stuff. IDK Im more into not making unnecessary repos if I dont have to.

I dont have the patience to handle either of those atm. 👅 🙄

[silverwind]

Gave it a quick test. Firstly, it does not seem to work in Firefox when opening a link like this one in a new tab, the active line is hidden below the headers. Secondly, I think the scroll offset should be set so the line comes exactly after the headers, currently there is a gap. Thirdly, I see this rendering issue:

May be better overall to move this sticky header and this hack to it's own repo, one less thing to worry about, and I don't really care about having it sticky.

[the-j0k3r]

Thirdly, I see this rendering issue:

its the .04em that maybe causing the rendering issue.

[silverwind]

I think you can do without specifying vertical padding, just specify horizontal only.

[the-j0k3r]

for me .04em is OK since .03em then I see a black underline

So idk.

[the-j0k3r]

I think you can do without specifying vertical padding, just specify horizontal only.

That makes the above issue more pronounced here, it was originally padding: 0 10px !important; and I couldnt get rid of that black line (which is something out of wack showing through)

I couldnt adjust that with pixels, lol this whole issue and everything surrounding it is somewhat insanity inducing..

[silverwind]

I didn't mean 0, but specifying them individually so the original value is unchanged:

padding-left: 10px !important;
padding-right: 10px !important;

But anyway, I will retest a better solution later.

[the-j0k3r]

this worked

padding-left: 1px !important;
padding-right: 10px !important;

but try this

  @supports (position: sticky) or (position: -webkit-sticky) {
    .pull-request-tab-content .diff-view .file-header {
      position: sticky !important;
      position: -webkit-sticky !important;
      top: 39px !important;
      z-index: 6 !important;
    }
    [id^=diff-].selected-line {
      position: relative !important;
      top: -120px !important;
      left: -10000px !important;
    }
    [id^=diff-].selected-line:before {
      content: attr(data-line-number) !important;
      position: absolute !important;
      top: 120px !important;
      left: 10000px !important;
      background-color: inherit !important;
      color: inherit !important;
			padding-right: 10px !important;
      box-sizing: border-box !important;
      width: 100% !important;
			height: 21px !important;
    }
    /* Queried from GitHub's js for the scroll offset - keep at 60px not more */
    /* see https://github.com/StylishThemes/GitHub-Dark/issues/598 */
    .js-sticky-offset-scroll {
      height: 60px !important;
    }
  }

[the-j0k3r]

May be better overall to move this sticky header and this hack to it's own repo, one less thing to worry about, and I don't really care about having it sticky.

I agree.

Firstly, it does not seem to work in Firefox when opening a link like this one in a new tab

Well no. you're right.

Secondly, I think the scroll offset should be set so the line comes exactly after the headers

Thats listed in cons. 2nd point.

the third see inline comment, I got good results with .03em but for some reason doesnt seem consistent.

[the-j0k3r]

Added the first issue @silverwind pointed out to cons.

[silverwind]

I'll play with this later, I'm sure a cleaner solution can be had.

[the-j0k3r]

I'll play with this later, I'm sure a cleaner solution can be had.

Thanks, I have full confidence in your skills, (famous last words) ;)

[the-j0k3r]

. Secondly, I think the scroll offset should be set so the line comes exactly after the headers

@silverwind, I agree its up there in the cons area, but if you click twice on same line, it will open in correct place see first and second con points.

[the-j0k3r]

Worth mentioning also the following @silverwind

Gave it a quick test. Firstly, it does not seem to work in Firefox when opening a link like this one in a new tab,

Also doesnt work in current master. lol