You need to detect window resize events

[istarkov]

And recalculate height or set dirty flag on window resize.

window.addEventListener('resize', this._onWindowResize);
// don't forget to unsubscribe
window.removeEventListener('resize', this._onWindowResize);

Also there must be some kind of refresh method for situations like next:
First component rendering occurs inside element with display:none style
a lot of tab controls use display:none style
Height is always be zero inside such elements.

So without some kind of manual refresh it will be impossible to use this component.

Good Work!!! I like this.

[nkbt]

Thanks, @istarkov

I will publish to bower asap so it is possible to create Codepen and easily reproduce issues.

Meanwhile if you can make an example or at least attach a gif (I recommend LiceCAP: http://www.cockos.com/licecap/) that would be awesome!

Though I have some doubts for now:

1. With display:none I believe it just should be covered in README. I have no intention to parse children and have any notion about what is happening there. This is a key feature of simple wrapper components I try to write.
2. For window resize what is the case? Width can change so content will have different height - yeah that is true. That should not be a problem since wrapper has height:auto and I doubt it is possible to resize window while animation is going. Well, you can do it of course, but you need to try really hard.

[istarkov]

1. You can check enormous amount of issues about such situation at my google-map component.
   It's not a rare situation.

2. For window resize what is the case?
   There is no need to resize window while animation going. You detect height of child element after first render. So for a lot of situations there will be incorrect height after any resize.
   And you get an animation jump at the animation end.

[nkbt]

1. I do believe it is a common case, but really do not want to touch children components. It should not be a concern of the react-collapse.
2. I get what you are saying, was just asking if you can show how it affects this lib. From what I can tell now, knowing the way it is implemented, there should not be a huge problem with window resize. Maybe some very awkward edgecase.

I can be mistaken, obviously, so would really appreciate reproducible example!

[istarkov]

1 - There is no need to touch anything - you just need to add method which allows recalculate internal height https://github.com/nkbt/react-collapse/blob/master/src/Collapse.js#L64

2 - Without any example it's easy to see that resize will be a problem. And the only way to solve it now is impossible without 1 or without resize event handler inside you code

:-)

[nkbt]

1. I am pretty sure that calling methods of your child component to recalculate anything is not a good idea. I mean in a situation like, how to call that method from outside? Passing isDirty prop?

<Collapse isOpened={true || false} isDirty={true}>
  <div>Random content</div>
</Collapse>

isDirty is an implementation detail that I would prefer to hide and never expose. I is basically a hack to allow me to dismount HeightReporter when it is not needed.

I guess the solution could be - expose height prop and if it is present - heavily simplify output. Like a static height animation. That was initially in my plan.

At the moment if content is always static with known height, I still do recalc, which is not optimal and can be easily fixed (keeping the same neat API)

2. Sorry I might be dumb but I can't easily spot the issue for now. I am using this component for my real project and can reproduce this. This is why I am asking for your help with example

[nkbt]

Created #2

By the way, I do appreciate your input, it is invaluable

[nkbt]

Just to clarify. When animation is completed, container height is set to auto, so it is not locked. This means if you resize window neither content nor wrapper is fixed so content is not cut off.

I guess that if you close amd open it again, height will stay previous, so animation will work till that initial height and then it will be expanded quickly to the actual height. Not a perfect effect but not extremely critical too, since all content will be visible.
I will definitely check it and if this suggestion is correct, will check for element width too. It should be better then resize since window resize does not necessarily mean component resize.

Thanks again for your comments.

[istarkov]

I understand you. But about not critical.

For me this is also a strange user behavior when she load page, play with controls, resize page, play with controls again etc.
But almost all of us have testers in our organizations.
Tester is not a normal user - she can click on the button with 20 clicks per second frequency, she can resize window, she can find a bug even on empty page ;-)
And it will be impossible to push commit with broken animation into master if tester find this.

[nkbt]

I've added another button for my example to change some stub text inside container. So it became possible to resize window and therefore resize element. As I expected there is nothing bad happening with content and animation.

The very little issue is there though, it was hard to spot, but I found it:

1. open container
2. resize window to change content's height
3. close container

When container starts closing animation it starts it from the old height, so there is a tiny little flicker at the beginning of animation, but rest goes smoothly. I'll have a look how to fix it without window resize checks (don't really want to listen to window resize event).

[sompylasar]

@nkbt it's very probable to have a window resize during animation on mobile, when the device is rotated. Slow your example animation down and you'll have plenty of time to simulate this on desktop.

[sompylasar]

@nkbt also have a look at handling element resize as a browser-generated event: http://www.backalleycoder.com/2013/03/18/cross-browser-event-based-element-resize-detection/

[nkbt]

@sompylasar thank you very much, I will definitely address this issue. At the moment it may make animation imperfect, though it is still not critical, since all the content has height: auto and will not be cut off. Did not pay enough attention to mobile rotate since I always have screen locked vertically and never rotate it, so missed this. Whatever the case, I am perfectionist, so can't live with known problems. Even potential ones.

In terms of perf, do you think it makes sense to expose something like fixedWidth prop (similar to existing fixedHeight, to avoid this additional measurement cost?

[sompylasar]

@nkbt for perf, look here for inspiration:
https://github.com/petehunt/react-touch-lib/blob/master/src/primitives/helpers/StaticContainer.js
https://github.com/petehunt/react-touch-lib/blob/master/src/primitives/AnimatableContainer.js

[nkbt]

@sompylasar thanks 👌

[souporserious]

I'd be willing to do a PR for this if you want. Did it with react-measure and it works well https://github.com/souporserious/react-measure/blob/master/src/Resize-Handler.js just add everything to one handler.

[nkbt]

1. I tried to rotate the phone while animating and found that lag between rotations is large enough for rotation to finish. When rotation is finished the hight becomes "auto", so it does not have any effect on the content. I'd like to see the example of broken height measure when window is resized (mobile rotated).

   Otherwise, I'd prefer not to add extra functionality that affects perf in a negative way.

2. It would also make ReactNative version (coming soon) incompatible without some extra effort.

3. The other concern is more ideological/architectural. Actually listening to something outside of the component. Listening to window resize feels more like an app concern and ideally should not be performed within a UI component.

These are the reasons this issue is still open. At the moment I am leaning towards "wontfix" and explain in README (Gotchas section) how this can be "fixed" (if needed) on app level. Or someone (me?) can make a wrapper component for Collapse, that uses Collapse, listens to window resize and updates props for Collapse so it is re-measured.

[souporserious]

Hmm then maybe expose a public method to calc height? Then someone can handle resize if they want to on their own. I had done that with React Measure in the early stages.

[nkbt]

You can use fixedHeight for that =) If you mean to let measure height from outside. Using fixedHeight height measurement is skipped entirely and you can do it from outside. Actually using same react-height, react-measure or any other tool.

Unless you meant to make Collapse recalc height (by internal means). In this case (at a workaround) you can toggle isOpened quickly like this.setState({isOpened: false}, () => this.setState({isOpened: true})), since height is recalculated on each opening.