nkbt / react-collapse

Component-wrapper for collapse animation with react-motion for elements with variable (and dynamic) height
MIT License
1.12k stars 113 forks source link

Position sticky inside collapse #309

Open iamcrisb opened 2 years ago

iamcrisb commented 2 years ago

Use case: Have one container for 3 divs(A,B,C) inside the collapse. Two of them(A,B) are being displayed based on a state property. Component C is a sticky one.

Whenever the state changes -> the collapse re-rendering causes a quick change between overflow: hidden and overflow: initial. The quick overflow change causes the sticky component (C) to flicker to offset and back.

No props are being changed on the collapse itself during this process.

nkbt commented 2 years ago

Do you have a reproducible case in codepen to play with?

iamcrisb commented 2 years ago

Do you have a reproducible case in codepen to play with?

I had no time to make one, but I already found a fix for it, I can do a pull request.

Fact is that the component was calling onResize on every component update, with having dynamic content in your collapse then overflow is switching back and forth so the sticky element flickers as well. I'm doing a check on my fork for

if (prevProps.isOpened !== this.props.isOpened) { this.onResize(); }

nkbt commented 2 years ago

How would I test it then?

iamcrisb commented 2 years ago

How would I test it then?

Yeah sorry, thought the explanation was pretty clear, I'll try to make a repo with it when I have time. Question is do you want the collapse to have a fluid growth with dynamic content inside it? If yes, then why? Shouldn't the developer handle that on it's own? IMO this component should only be applying overflow hidden when the state changes from open to closed.

Also it is a performance issue if you call the resize function on each update.

nkbt commented 2 years ago

Don't think I get your point. Collapse container simply follows the content. When content changes and change is detected, collapse will update the container height and after container reaches target height - switch to "stable" mode (overflow visible, height auto). That's all it does really.

iamcrisb commented 2 years ago

All right, got ya. Just wanted to make sure that this is how you want it to work. In this case, we need to find a way of doing it in order to avoid this

The element is positioned according to the normal flow of the document, and then offset relative to its nearest scrolling ancestor and containing block nearest block-level ancestor, including table-related elements, based on the values of top, right, bottom, and left. The offset does not affect the position of any other elements.

This value always creates a new stacking context. Note that a sticky element "sticks" to its nearest ancestor that has a "scrolling mechanism" (created when overflow is hidden, scroll, auto, or overlay), even if that ancestor isn't the nearest actually scrolling ancestor.

https://developer.mozilla.org/en-US/docs/Web/CSS/position

Doing this inside the work function at every timeout causes a flickering for a position sticky inside collapse this.container.style.overflow = "hidden";

nkbt commented 2 years ago

I suggest not to use position: sticky inside collapse container. Sticky is meant to be used with scrollable elements. And it does change that element internal height while doing its magic. Also cannot possibly imagine having container with height: auto that needs sticky element. If sticky element is used that means container has some scroll in it, and that means it has some known height. In this case collapse is completely unnecessary as its whole purpose is to make vertical animation for elements with unknown target height (px -> auto, em -> auto, etc). If height change is known (px -> px, em -> px, etc) then simple css transition would just work fine.

iamcrisb commented 2 years ago

If sticky element is used that means container has some scroll in it, and that means it has some known height.

Hmm, not true, you can have content that is loading async when opening the collapse and then you don’t know the height.

I wanted to use it in a section on a landing page that basically contains an mvp for an app.

anyway, seems that it is beyond the scope of the lib, I will try to find some time and make a pull request for sticky. Meanwhile I think we can add a note in the project description for people to know about weird component interaction with position sticky.