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

Broken scroll element behaivior #612

Closed
the-j0k3r opened this issue May 7, 2018 · 25 comments
Closed

Broken scroll element behaivior #612

the-j0k3r opened this issue May 7, 2018 · 25 comments

Comments

@the-j0k3r
Copy link
Member

the-j0k3r commented May 7, 2018

Take a Poll



The first file diff stat header (see the the screenshot) gets dragged along the bottom when scrolling, this doesnt happen when GH-Dark is off, Ive taken a gif as well to illustrate.

capture

  • Browser: FF 59.0.3
  • Operating
    System
    : Windows 10 1709
  • Screenshot:

selectors

https://github.com/travis-ci/travis-web/pull/1644/files

  • HTML of the section where the issue occurs:
<div class="file-info">
        <span class="diffstat tooltipped tooltipped-e" aria-label="1 addition &amp; 1 deletion">2 <span class="block-diff-added"></span><span class="block-diff-deleted"></span><span class="block-diff-neutral"></span><span class="block-diff-neutral"></span><span class="block-diff-neutral"></span></span>

      <a href="#diff-87cf2f988c8b885dcfc13394b0335ce0" class="link-gray-dark" title="app/styles/app/layouts/error.scss">app/styles/app/layouts/error.scss</a>

      
    </div>
@Mottie
Copy link
Member

Mottie commented May 7, 2018

Hi @the-j0k3r!

I'm not sure what you mean by the header being broken. The file header was made sticky intentionally so you can see the name of the file you're viewing. This also works with the GitHub Fixed Header userstyle.

@the-j0k3r
Copy link
Member Author

the-j0k3r commented May 7, 2018

As you see the file-info section is being knocked down and stuck as you scroll, if youre saying that the file-info is intentionally stuck, then why doesnt it scroll along the same way when GitHub Dark is turned off? it looks broken.

Edit: Its in the gif, idk how else to explain it, the file-info which is the header part of the first commit is dragged along, this looks broken, Im not talking about this area.

capture

Im talking about

capture

^ that should not be stuck, right?

@Mottie
Copy link
Member

Mottie commented May 7, 2018

I'm saying that GitHub-Dark intentionally makes each file-info section header stick while scrolling, so you know which file diff you're looking at. It unsticks when the bottom of the file block (e.g. error.scss reaches the bottom of the "Changes from all commits" header.

Edit: Here is it in the style:

  /* copied from Refined GitHub extension */
.pull-request-tab-content .diff-view .file-header {
  position: sticky !important;
  position: -webkit-sticky !important;
  top: 40px !important;
  z-index: 6 !important;
}

@silverwind
Copy link
Member

Looks working as intended, thought one could argue that we shouldn't do that stickying and leave it to other extensions.

@the-j0k3r
Copy link
Member Author

the-j0k3r commented May 7, 2018

Sorry looks very odd and wrong in comparison to unstyled, should be optional at best.

  .pull-request-tab-content .diff-view {
    position: sticky !important;
    position: -webkit-sticky !important;
    top: 40px !important;
    z-index: 6 !important;
  }

I can confirm that that works same as unstyled removing that .file-header makes this nuisance go away.

@Mottie
Copy link
Member

Mottie commented May 7, 2018

We don't have many opinionated additions in our GitHub-Dark style. We could make this feature optional, but you're the first to call it a nuisance. If we get more people requesting to make it an option, then we'll go ahead and make the change.

For now, you can add a new style to Stylus/Stylish to override this behavior:

/* ==UserStyle==
@name        GitHub-Tweaks
@namespace   https://github.com/StylishThemes/GitHub-Dark/issues/612
@version     1.0.0
@description Tweaks
@author      Me
==/UserStyle== */

@-moz-document domain("github.com") {
  body .pull-request-tab-content .diff-view .file-header {
    position: static !important;
  }
}

@xt0rted
Copy link
Member

xt0rted commented May 8, 2018

I think this was done as a fix to allow github dark to work with refined github #531.

@the-j0k3r
Copy link
Member Author

the-j0k3r commented May 8, 2018

I think this was done as a fix to allow github dark to work with refined github #531.

I see....

For now, you can add a new style to Stylus/Stylish to override this behavior

Its not the end of the world, thx for the override.

We don't have many opinionated additions in our GitHub-Dark style. We could make this feature optional, but you're the first to call it a nuisance.

Im not sure being that first is necessarily a good thing 🙄 with some luck i'll get used to it,

What is annoying is that there is a gap between the default sticky bar and this, so in the dark style whatever is color/bright behind that gap is scrolling away, its... distracting my eyes get drawn to it and not where my focus should be,

Design wise Im not sure how else to incorporate that "feature" into the bar above to only have one sticky that updates this as you scroll leaving the other untouched.
That or remove that gap.

@Mottie
Copy link
Member

Mottie commented May 8, 2018

Oh, odd, I'm not seeing the gap... I see in the screenshot now. What browser are you using? I've noticed that in Chrome, if I use zoom levels other than 100%, sometimes gaps appear, especially on bordered items. If we set the top to 39px, would that fix the issue?

@xt0rted
Copy link
Member

xt0rted commented May 8, 2018

I see it faintly in FF (goes away once I stop scrolling). Little things like this happen pretty regularly if you're using any form of scaling, which I am. I have 150% on the OS level with 4k displays.

Chrome seems to render the page faster so you don't notice the slight gap which is one of the reasons I switched to it. Edge & FF tend to be slower and show more of these issues in my experience.

The issue goes away in FF for me if I switch to 39px.

@the-j0k3r
Copy link
Member Author

the-j0k3r commented May 8, 2018

Oh, odd, I'm not seeing the gap... I see in the screenshot now. What browser are you using? I've noticed that in Chrome, if I use zoom levels other than 100%, some times gaps appear, especially on bordered items. If we set the top to 39px, would that fix the issue?

No scaling, but Im on a 4k screen with 2560 x 1440p resolution, browser is FF 59.0.3 as initial reported.

top: 39px works fine ;) so there! Color me happy.

OFFTOPIC

I been chugging at a travis CI dark theme, coverage is 99.99% on all navigable pages/sites, just have a couple of issues to try get rid of and should be good to share (despite not being happy about one or two things)

@Mottie
Copy link
Member

Mottie commented May 8, 2018

Updated in the master branch... it'll be available the next time we push a release.

@the-j0k3r
Copy link
Member Author

Thanks for indulging my opinionated and first time nuisance badger. 1px fix 🤦‍♂️

@the-j0k3r
Copy link
Member Author

I have a question about stylish website where you submit these, if you added an option tick box to add this tweak and create a second Code portion is Stylus, would that work?

If so the tabs and everything could all be done as additions to main or no?

@Mottie
Copy link
Member

Mottie commented May 10, 2018

Yes, we could add this sticky header behavior optionally. It would require:

  • Replacing the sticky definition in the style with a placeholder.
  • Updating the Gruntfile.js file:
    • Custom build: Check the build.json for the new setting and replacing it in the core style as needed.
    • grunt user build for userstyles.org pasting - nothing to do here...
  • Add the option to the usercss meta data
  • Adding the option to userstyles.org.
  • Updating the GitHub Dark userscript:
    • Add option setting to config panel.
    • Replace the placeholder with the user selected setting.

It's not as much work as it appears, but you are literally the only person to ever as for this to be removed; so, I'd like to wait for some more feedback from other users.

@Mottie
Copy link
Member

Mottie commented May 10, 2018

I added a poll to the first post...

@the-j0k3r
Copy link
Member Author

the-j0k3r commented May 10, 2018

Not removed,, you could make it ON by default, and optionally OFF, it was just a suggestion.

Now that the gap is gone, this isnt a nuisance anymore and the above suggestion/question is literally because you have asked for others opinions and not closed this.

If you were to close it, or mark as enhancement type thing depending on the route you think is best, that would be OK with me also. :)

Anywho, thanks, you guys do the best styles (2726 ppl including me think so), something Im striving for with my first coming out party. on the Travis CI Dark complete with all websites associated by navigation.

@the-j0k3r
Copy link
Member Author

the-j0k3r commented May 10, 2018

I added a poll to the first post...

nice, but all I see is two bars ;)

capture

So I clicked on the first bar just as a test because... How do I know which bar is for what?... Looks lovely and I may have just voted for Trump as a world leader... OMG... please no....

EDIT, I see, I see, the poll label is really really faint with GitHub-Dark on, So shall I open bug for that :D ha!

hashtag fail hashtag smoke it.

@the-j0k3r
Copy link
Member Author

Just found 2 bugs on that polling API.

  1. Lets you vote more than once
  2. Even when it spits out errors a refresh is all it takes...

@Mottie
Copy link
Member

Mottie commented May 10, 2018

Report the poll issues, here.

I'll work on inverting the image properly.

@the-j0k3r
Copy link
Member Author

Somewhere in #598 is a good case for making this optional.

@the-j0k3r
Copy link
Member Author

the-j0k3r commented Jul 20, 2018

@Mottie @silverwind now would be a good time to revisit this issue. Since #598 has been resolved and you have a good stable base for either leaving as is or making it an optional style,

I originally had voted to make this optional, but am now more inclined to leaving it as is since its causes no issues.

After last night and after using the final solution for #598 for a while now I vote now to leave as is if were looking at votes though my chnage of heart as stands now counts for naught.

I would like to close this one way or another :)

@silverwind
Copy link
Member

I'm actually surprised on the amount of votes in that poll. It seems rigged :)

@the-j0k3r
Copy link
Member Author

the-j0k3r commented Jul 20, 2018

Short answer, I agree with you.

TLDR

Ill be honest, originally because the poll wasnt visible while GHD was on, I clicked randomly twice and it the votes were cast to leave as is (turning off GHD it was plain to see and I got two thanks for voting screens) before I posted about that, So then cast vote to balance on the opposite direction which was my original intention.. 3 votes to make optional so it was one more than the opposite (as if it was just started)

Then I found this multi-vote wasnt right and said it

Edit: see bug report just opened apex/gh-polls#38

But as for my part I left the votes alone to balance out my blind votes.

Now before making my last comment I voted to leave as is again once,so while it may seem rigged Im not responsible for the remainder 7868.

Technically that makes 1 vote for and 1 against in any case .

But since it was made clear there is a bug, who knows who else could have abused the system, proving like you said, its likely rigged :)

@the-j0k3r
Copy link
Member Author

closing due to 65f3e82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants