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

Use scroll-margin with floating navigation #7528

Merged
merged 2 commits into from
Mar 30, 2020

Conversation

jonathantneal
Copy link
Contributor

@jonathantneal jonathantneal commented Mar 28, 2020

This changes fragment links to use scroll-margin-top to offset their vertical scroll position when experienced alongside the floating navigation.

This also changes the vertical offset from 75px to calc(64px + 0.5em). The 64px accounts for the height of the SubNav, which is computed from px units. The 0.5em accounts for the visual breathing room of the target itself, which is often text and thusly defined by its em size.

Here are some examples of how the px and em work best together:

This change also updates the CSS selector used for this effect so that it applies to all active targets that appear after the SubNav. This allows non-link destinations like the Sidebar or "Edit this page" button to receive the same treatment, as well as any links that may not conform to the exact DOM tree presently required.

Note: If the Sidebar "Filter" <input> had a name or id, you would also see that a link to that fragment would correctly scroll the page and focus the input.

Coincidenting benefits

The use of the :target selector clearly communicates the intent of the rule while reducing the present complexity of the selector.

The use of a scroll-margin property clearly communicates the intent of the declaration, while the removal of the position and its related styles reduce layout complexity.

The scroll-margin property is not supported by Internet Explorer 11, which is a good thing because IE11 users do not experience the sticky navigation either. This prevents the unideal situation where an IE11 visitor is presented the wrong definition on a page, because the correct definition happens to be 75px below it.

@jonathantneal jonathantneal changed the title Use scroll-margin to offset link positions when navigation is sticky Use scroll-margin to offset links when the navigation is sticky Mar 28, 2020
@jonathantneal jonathantneal force-pushed the jn.alternative-link-margin branch 3 times, most recently from 8651f15 to 3fe2e21 Compare March 28, 2020 04:29
@jonathantneal jonathantneal changed the title Use scroll-margin to offset links when the navigation is sticky Use scroll-margin when the navigation is sticky Mar 28, 2020
@jonathantneal jonathantneal force-pushed the jn.alternative-link-margin branch 2 times, most recently from 5bdd00d to 7be741e Compare March 28, 2020 04:59
@jonathantneal jonathantneal changed the title Use scroll-margin when the navigation is sticky Use scroll-margin with floating navigation Mar 28, 2020
@jonathantneal jonathantneal marked this pull request as ready for review March 28, 2020 05:27
@jonathantneal jonathantneal force-pushed the jn.alternative-link-margin branch from 7be741e to 73a09f2 Compare March 28, 2020 05:36
Copy link
Contributor

@jescalan jescalan left a comment

Choose a reason for hiding this comment

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

We spoke about this on slack, but would love a little more clarity in a code comment here. Re-request me when it's done and we'll be ready to roll!

@jonathantneal
Copy link
Contributor Author

Definitely, thanks @jescalan! Done in f2015b9.

@jonathantneal jonathantneal requested a review from jescalan March 30, 2020 18:11
@jonathantneal jonathantneal merged commit 53c578d into master Mar 30, 2020
@jonathantneal jonathantneal deleted the jn.alternative-link-margin branch March 30, 2020 19:04
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants