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-top instead of pseudo hack #30273

Merged
merged 4 commits into from Mar 12, 2020

Conversation

MartijnCuppens
Copy link
Member

I recently discovered the scroll-margin-top property and it turned out this was exactly what we needed instead of the hack we have. Some side notes:

  • This doesn't work for IE, but since sticky itself doesn't work, this is a non-issue.
  • This doesn't work for the old Edge, I'm not sure if that's really an issue since it's just for our own docs.

I've also wrapped it in a media query since the stick menu is only present above the md breakpoint.

site/assets/scss/_content.scss Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

Not working in Edge is an issue for sure. Not going to block it though, since we are talking about a small percentage anyway https://caniuse.com/#feat=mdn-css_properties_scroll-margin-top

@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Feb 24, 2020

Not working in Edge is an issue for sure.

It just doesn't work in the old edge, caniuse is wrong here. (seems to be fixed recently, but not yet updated on production mdn/browser-compat-data#5557)

@XhmikosR
Copy link
Member

I personally meant the old Edge. Like I have mentioned in the past, browsers don't magically get updated on Windows :P Also, Microsoft hasn't attempted to replace the old Edge yet, let alone that even if they do, there are so many supported Windows versions out there.

Anyway, this is just a FYI, ideally we should have some kind of fallback, but even without it I guess we could try landing it later.

@mdo mdo added this to Inbox in v5 via automation Mar 8, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Mar 9, 2020

Can't we have a fallback of some kind?

@ffoodd
Copy link
Member

ffoodd commented Mar 12, 2020

@XhmikosR by using @supports(), it should be pretty easy — however it means more code.

@MartijnCuppens
Copy link
Member Author

@XhmikosR by using @supports(), it should be pretty easy — however it means more code.

This would be moot. The current implementation works 100% (but is pretty ugly), no reason to just add code.

@XhmikosR
Copy link
Member

Technically, this does not work 100% since it doesn't work in all of our supported browsers.

Anyway, #30273 (comment)

v5 automation moved this from Inbox to Approved Mar 12, 2020
@XhmikosR XhmikosR merged commit 3b555aa into master Mar 12, 2020
v5 automation moved this from Approved to Shipped Mar 12, 2020
@XhmikosR XhmikosR deleted the master-mc-scroll-margin-top branch March 12, 2020 15:56
@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Mar 12, 2020

I feel the day I can convince @XhmikosR to fully drop legacy Edge support is coming closer...

@ffoodd
Copy link
Member

ffoodd commented Mar 13, 2020

Just to be sure, couldn't this be applied to sticky / fixed navbars component too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

4 participants