gm0t / react-sticky-el

MIT License
251 stars 47 forks source link

ScrollElement top distance not taken into account on hideOnBoundaryHit #67

Closed danielfdsilva closed 3 years ago

danielfdsilva commented 3 years ago

If there is some distance between the scrollElement and the top of the viewport, the sticky element will jump to the top of the page when hitting the boundary.

This is happening here: https://github.com/gm0t/react-sticky-el/blob/e4c99e204017ce5b1aef92d9d1fab4321ccdca7a/src/render-props-version.js#L35-L37

The value for top will end up being something like -Xpx and with position: fixed this refers to the viewport.

This can be replicated by adding a margin to the scrollElement on the hideOnBoundaryHit > false example.

It seems that taking the top value into account would solve the problem, but I'm not sure of the implications:

{ top: `${boundaryBottom - height - bottomOffset}px` }
// simplifying { top: `${top + boundaryBottom - (top + height + bottomOffset)}px` }
gm0t commented 3 years ago

Thanks! I will try to take a look at this one over the weekend :)

gm0t commented 3 years ago

@danielfdsilva could you please tell me a bit more about your use-cases, how did you run into this problem? I tried your idea - it makes things a bit better, but still does not really solve the problem : image

And I'm not entirely sure that this problem is fixable, because the scrollarea has to have overflow-y: scroll and that will not allow us to hide this sticky element :(

danielfdsilva commented 3 years ago

@gm0t In my case the page has an header, and the scollarea is below it, and only that area is scrollable - the header stays put. I imagine the overflow problem could be solved if the scrollareacontainer has overflow: hidden, no?

gm0t commented 3 years ago

@danielfdsilva if you apply overflow: hidden you will lose your scrollarea :) I would recommend you to have a wrapper around your scrollarea and apply a margin on that wrapper, then everything should work just fine. Something like this:

<header>header</header>
<content style="margin-top: 100px;">
  <div className="scrollarea">
    content with sticky el
  </div>
</content>

What would you say?

danielfdsilva commented 3 years ago

@gm0t I meant applying the overflow: hidden to the wrapper. In your example would be <content>. In that way you wouldn't lose the scroll.

What you mention is exactly what I did, and what lead me to the problem. In your storybook example you have:

<div class="container">
  <div class="column">
    <div class="scroll-area">
      Content
    </div>
  </div>
</div>

Applying a margin to either column or container results in the problem I'm describing.

gm0t commented 3 years ago

yeah, unfortunately, that's not going to work either due to position: fixed being applied at that time I think, that position: fixed is the problem in this case... it should've been replaced with absolute when reaching that state - then everything will work fine

danielfdsilva commented 3 years ago

@gm0t 👋 Got pulled into other things and did not finish this conversation.

I kind of like the fact that position: fixed is being used. In this way you don't have to worry about the position of other elements. If this were using position: absolute and a parent element had a relative position it would mess things up. This would be solved by portalling the sticky element to the body and manually position it, but this adds another level of complexity.

I think that the suggestion I gave is still valid as it solves the jumping to top problem. True that you're left with the element overflow problem, but that is already present.

The overflow can be "solved" by using clip-path: content-box; on the scrollarea element. This may have other implications but the user should evaluate them on a use-case basis. Thoughts?

gm0t commented 3 years ago

@danielfdsilva that sounds like a plan, would you mind creating a PR?