issues: sticky hdr covers selected line fixes #598

[the-j0k3r]

This uncovers just enough of the selected line to be a good candidate to fix #598

Im using this now locally and it works reliably enough to warrant this PR.

Re: non-default sticky header styles/scripts,

If there is a problem with those, it is beyond the scope of this issue and this fix, IMO and they need to be addressed separately and should not be a blocker for anything like this.

@maxrothman since you were the person to report the issue maybe this would be a good enough solution rather than none or having to do js workaround scripts.

[silverwind]

Hmm, can you uncover maybe 2 pixels more through reducing the padding on the stickied header? Note that I'm generally against stickying this header, but I see the issue is rather complicated.

[the-j0k3r]

Not really, try it yourself it will get hairy really quick after this and it wont look nice when its not stuck.

[the-j0k3r]

I was just giving this more thought, perhaps theres a better solution, but it will probably double the amount of code, Im wondering if you can make it look one way when not stuck and another when stuck by using these @support rules.

I'll play around for the sake of interest in learning a new trick.

[silverwind]

Let's not add too many hacks, I'll see what can be done about compat with refined-github and if a user wants these sticky headers, they should install that extension. The issue with the out-of-view linked lines is also present with it, by the way.

[the-j0k3r]

I dont think what I was thinking is going to work.

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: 35px !important;
      z-index: 6 !important;
    }
    .pull-request-tab-content .file-header {
      height: 39px !important;
    }
    .pull-request-tab-content .diff-view .file-actions,
    .pull-request-tab-content .diff-view .file-info {
      margin-top: -1px !important;
    }
  }

if you're happy with that Ill push it instead.

edit...

thats a magnified screenie.

[silverwind]

See #701 for my idea of not stickying at all in the style.

[the-j0k3r]

As I commented there, the code in my comment above yours is the best compromise between stuck/unstuck I could find, and Im used to the idea and find that sort of useful.

A third solution is to make my amended proposal entirely optional. as long as it works with #701 in same way. I know its a big can of worms.

[the-j0k3r]

Let me know if you want the code as is or if you want the revised on the comment above. Im around for another 10 minutes before I go take a break ;)

[silverwind]

This is how this PR looks on my screen:

With top: 34px it would line up perfectly with the line for me. If I then make .pull-request-tab-content .file-header also 34px high, it looks almost alright, except the button is slightly off-center vertically. What do you think?

[the-j0k3r]

34px is too much, if you consider eagle eyes at stuck/unstuck mode

with

  @supports (position: sticky) or (position: -webkit-sticky) {
    .pull-request-tab-content .diff-view .file-header {
      position: sticky !important;
      position: -webkit-sticky !important;
      top: 35px !important;
      z-index: 6 !important;
    }
    .pull-request-tab-content .file-header {
      height: 39px !important;
    }
    .pull-request-tab-content .diff-view .file-actions,
    .pull-request-tab-content .diff-view .file-info {
      margin-top: -1px !important;
    }
  }

looks like

[the-j0k3r]

Ive pushed the tweak.

[silverwind]

top: 35px and height: 36px looks fine on my screen (height: 39px leaves a slightly off-center button).

[the-j0k3r]

thats the problem, your 36 height kills the centering on unstuck,

Ive played with this for a long time now to find a good balance between stuck/unstuck and centering on both.

[silverwind]

Try this:

.pull-request-tab-content .diff-view .file-header {
  top: 40px !important;
}
.pull-request-tab-content .file-header {
  height: 32px !important;
  padding: 1px 10px;
}

[the-j0k3r]

Nope, 40px leaves the 1px gap and around we go to reason why I opened #612 :/

[the-j0k3r]

well idk... Ill push that

[silverwind]

Well then make it 39px, still looks alright for me, but I don't see that gap you mentioned.

[the-j0k3r]

pushed...

RGH eat our shorts :)

[silverwind]

Merged in 9b5fa7e with followup tweak in 5743cd7.

[the-j0k3r]

Again you broke it with your tweak

[the-j0k3r]

@silverwind you closed it, is that cause my commit count is getting to high? JK lol

[silverwind]

No, it's just GitHub being too dumb to detect CLI merges :)

[the-j0k3r]

EH it also strips everyone's lovely signature on PR merge via rebase/squash :(

CLI merge you need to pull my branch before, which is convoluted also. I never got that process when the PR already implies that its been pushed.

[silverwind]

I don't mess with branches usually, I just use apply-pr.

[the-j0k3r]

Ah not buggy then :) 3yr old tool :)

Youde be better served with Git aliases