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

[UI] Fix scrollbar overlapse bug #1540

Merged
merged 3 commits into from Feb 15, 2019

Conversation

maestromac
Copy link
Member

What type of PR is this? (check all applicable)

  • Bug Fix

Description

Removed two CSS line to fix a UI bug. Will need another pair of eyes to verify I didn't break anything somewhere else though.

Related Tickets & Documents

Resolves #1332

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

before

After
screen shot 2019-01-14 at 4 24 10 pm

Added to documentation?

  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

alt_text

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 14, 2019
Copy link
Contributor

@Link2Twenty Link2Twenty left a comment

Choose a reason for hiding this comment

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

This fixes the problem.
I believe the reason this was done was so the anchor point would start you below the nav bar.
If this change were to be applied you would start with the anchor point at top 0 which is behind the nav.

Here is an example with and without the padding/margin hack (and anchor links just to make it easier to show)

With:
anchor_hack

Without:
anchor_no_hack

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 15, 2019
@Link2Twenty
Copy link
Contributor

Link2Twenty commented Jan 15, 2019

While we're here, how do we feel about adding a link icon after headers?

Maybe something like this?

http://jsfiddle.net/link2twenty/4Lxo2e7a/

Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

As @Link2Twenty mentioned, the padding-top and margin-top were to allow the headers to jump properly and account for the navbar. We might be able to solve this with z-index and allowing the code block to be on top of the header element.

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 15, 2019
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Jan 15, 2019
@maestromac
Copy link
Member Author

I knew I broke something.

@Zhao-Andy
Copy link
Contributor

Also @Link2Twenty I think it makes sense for this PR to only fix the bug. Like the use of an emoji for the icon though.

Would you like to open a feature request issue for the icon? I think it makes sense, too; there are some implementation details we'll want to discuss that probably should be left in a separate issue.

@benhalpern
Copy link
Contributor

@maestromac @Zhao-Andy @Link2Twenty I think this can be fixed most simply by making the z-index of the code area higher than the z-index of the header, eh?

With the link thing: I think the left margin is still my favorite place for it. And possibly only on hover?

We'd previously had an implementation of this idea which shipped the image in the actual HTML which didn't seem right. I think ideally this would maybe be something included in the CSS ::before or something along those lines. Does that make sense?

@maestromac
Copy link
Member Author

@benhalpern I've toyed around with adjusting z-index in this commit but it doesn't seem to work. Maybe I did it wrong? This error is reproducible on Firefox 64 on MacOS (just make sure your scrollbar are always displayed

Copy link
Contributor

@Link2Twenty Link2Twenty left a comment

Choose a reason for hiding this comment

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

&.anchor{
  position: relative;
  top: -50px;
  display: block;
  visibility: hidden;
}

Having just done a couple of tests, admittedly in chrome, this should be enough.

@Link2Twenty
Copy link
Contributor

@Zhao-Andy @benhalpern I'll raise a feature request in the morning, for the anchor links, but it sounds like Ben is hoping for something like this.

http://jsfiddle.net/link2twenty/2z7rqu1k/

Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Just tested with Mac, it works across macOS Chrome, Firefox, Safari and Windows 10 Chrome, Firefox, and IE. Since it's an old PR I think we should favor merging it over small changes.

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 15, 2019
@maestromac maestromac merged commit 91848f0 into forem:master Feb 15, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 15, 2019
@maestromac maestromac deleted the bug-ui/#1332-scroll-bar-overlapse branch February 15, 2019 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header following a code block - scrollbar overlapse
4 participants