-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add support for scroll-margin and scroll-padding #386
Comments
Hi! I've been wondering about how to implement this. Trying to get things to scroll correctly with a sticky header has been uneven, and I like the idea of supporting these properties to achieve consistency. Here's a rundown of my thoughts so far; I'd love your input if you have time! Things affectedScroll padding and margin would affect two things:
Communicating those propertiesReading the spec, it seems that eventually we'd be able to read the computed style, same as currently done in compute-scroll-into-view. Until browsers implement it, that seems unlikely. Similarly, as a polyfill, I think this means we have to pass those properties as function arguments? Code bitsIf the above is true, it seems that we'd have to modify both scroll- and compute-into-view. I see perhaps how |
Hey @fpapado! I appreciate your thoughtful feedback! I'll do my best to respond here. When I set out to refactor this package into what became That's why the I haven't dived into the spec on
You're also right that reading these new CSS property can prove to be a challenge. Unlike border widths it's unlikely they'll show up in I really hope we can avoid needing to pass these values as properties in function call itself. Especially since today's API don't let you specify other elements than the target element and the boundary. Specifying scroll margins and paddings on elements between the boundary and scroll target would require the API to be much more complex. I hope it's possible to somehow have it stay in CSS so that the migration path away from this polyfill stays is as simple as: Another question is wether Since the bundlesize of downshift is directly affected by this I'd like to hear what @kentcdodds thinks, and perhaps ask for feedback from the downshift community as well 🙂 |
How much will this impact the bundle size? |
I don't know yet what the impact will be. I'll run a POC and ping you when I have some numbers to look at 🙂 |
Hey, @stipsan! Loved that response, it matches much of my own dilemmas when considering where and how this could be supported. I think exposing hooks in A random thought that came to mind: could we use a custom property as a halfway point between the "real" CSS properties and the ponyfill? Those would show up in I need to think a bit more about the other constraints, but I'm hopeful! |
A |
The best way to support IE11 is to use a markup structure that can accomplish the same things without using With that in mind I think it's fine to rely on custom properties to "bridge the gap". As in we can use Again, this would mean you can't use this feature if you need to support IE11. But I don't believe we can make it work in IE11 without ending up with an unacceptable bundle size or impractical complexity. With regards to making standards cry... I'd rather use custom properties than introduce even more properties in the JS API that isn't in the spec if you know what I mean 😅 |
I need either this, or an |
The draft spec has signaled this is coming in the if-needed spec, I'll work on this soon 😄 |
Hi @stipsan :). Is there any progress on supporting scroll-padding/margin to this library? It seems to work pretty well, but we have a use case where we need to actually "flush" the container to fill the whole width, and adding the scroll margin/padding is currently necessary. |
Hi @stipsan , |
I also needed this. (Chrome supports scroll-padding with scrollIntoView but has some bugs preventing me to use it.) Here's my implementation (supporting top and bottom padding). For scrollPaddingTop, we can just subtract it from the computed top. For scrollPaddingBottom, we then need to make sure that the computed top doesn't put the target too low. Would be happy to see support for this in import { compute } from "compute-scroll-into-view";
const getScrollPaddings = (target: Element) => {
const computedStyle = window.getComputedStyle(target);
return {
top: parseFloat(computedStyle.scrollPaddingTop) || 0,
right: parseFloat(computedStyle.scrollPaddingRight) || 0,
bottom: parseFloat(computedStyle.scrollPaddingBottom) || 0,
left: parseFloat(computedStyle.scrollPaddingLeft) || 0,
};
};
export function scrollIntoView<T = unknown>(
target: HTMLElement,
options: Parameters<typeof compute>[1] & { behavior: ScrollBehavior },
): T | void {
for (const { el, top, left } of compute(target, options)) {
// Get scrolling element's scroll-padding
const scrollPaddings = getScrollPaddings(el);
// Subtract padding from computed top
const topWithPadding = top - scrollPaddings.top;
// Make sure to not scroll too far down
const safeMin =
target.offsetTop +
target.getBoundingClientRect().height +
scrollPaddings.bottom -
el.clientHeight;
const adjustedTop = Math.max(topWithPadding, safeMin);
el.scroll({ top: adjustedTop, left, behavior: options?.behavior });
}
} |
Documented in this spec: https://drafts.csswg.org/css-scroll-snap-1/#propdef-scroll-margin
This is how it'll be possible to set custom offsets without reimplementing the default scrolling behavior or changing the HTML structure to add wrapping elements with padding to accomplish the same thing.
The text was updated successfully, but these errors were encountered: