Skip to content
This repository was archived by the owner on Mar 14, 2024. It is now read-only.

fix scroll-position for in-page links relative to fixed header - #2762

Merged
robdodson merged 3 commits into
masterfrom
scrollpadding
Jun 8, 2020
Merged

fix scroll-position for in-page links relative to fixed header#2762
robdodson merged 3 commits into
masterfrom
scrollpadding

Conversation

@paulirish

Copy link
Copy Markdown
Member

THIS CSS IS AMAZING

html {
  scroll-padding-top: 70px; /* height of sticky header */
}

https://css-tricks.com/fixed-headers-on-page-links-and-overlapping-content-oh-my/

Fixes #2598

cc @brendankenny
adding some keywords because i always have trouble refinding this property: scroll under overlap offset sticky fixed header named anchors inpage hash

@googlebot googlebot added the cla: yes Contributor has signed the CLA label May 1, 2020
Comment thread src/styles/generic/_shared.scss Outdated

// https://css-tricks.com/fixed-headers-on-page-links-and-overlapping-content-oh-my/
html {
scroll-padding-top: calc(#{$TOPNAV_HEIGHT} + 24px);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

24 was arbitrary but it felt nice to give it a tad more space. otherwise the text is right up against the bottom of the header.

@netlify

netlify Bot commented May 1, 2020

Copy link
Copy Markdown

Deploy preview for web-dev-staging ready!

Built with commit ea22c72

https://deploy-preview-2762--web-dev-staging.netlify.app

@samthor

samthor commented May 1, 2020

Copy link
Copy Markdown
Contributor

IIRC we're putting a margin on headings to emulate this experience (margin-top: -$TOPNAV_HEIGHT; padding-top: $TOPNAV_HEIGHT). Can we remove that if this works?

@robdodson

Copy link
Copy Markdown
Contributor

IIRC we're putting a margin on headings to emulate this experience (margin-top: -$TOPNAV_HEIGHT; padding-top: $TOPNAV_HEIGHT). Can we remove that if this works?

Yeah I would love to remove that and swap in this CSS. I had thought that this feature wasn't well supported but it seems to be. I'm not sure I understand the bit about Safari not supporting it on "fragment targets".. isn't that what we're linking to? But we can test it in Safari and see :D

@robdodson

Copy link
Copy Markdown
Contributor

hm yeah it doesn't seem to work in safari :(

@robdodson

Copy link
Copy Markdown
Contributor

to test, remove lines 74+ in _shared.scss

@robdodson

Copy link
Copy Markdown
Contributor

What if we do something like this?

https://twitter.com/simevidas/status/1257376146523652098

@stale

stale Bot commented Jun 3, 2020

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. To prevent his from happening, leave a comment.

@stale stale Bot added the stale label Jun 3, 2020
@robdodson

Copy link
Copy Markdown
Contributor

Technically Safari does support scroll-padding-top but only inside of a scroll snap container. Unfortunately that means we can't do:

@supports(...) {}
@supports not (...) {}

@stale stale Bot removed the stale label Jun 3, 2020
@robdodson

Copy link
Copy Markdown
Contributor

Sooooooo I rebased the PR to just make our anchor padding hack have a bit more space because I can't figure out a good way to feature detect scroll-padding. But I would like to merge the change I made at least.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes Contributor has signed the CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Viewports off for internal links

4 participants