respect prop style object overflow property

[Jonathan-Millar]

Currently if the css property overflow is passed in the style object it is stepped on and not respected, resulting in fixed height containers not being scrollable. This moves the style spread to after defaults to ensure it is respected,

[nkbt]

Hi, @Jonathan-Millar!

First of all, thank you for your contribution!

Now to the change itself =)

It was actually intentional, otherwise it is possible to override overflow. Can you give an example of doing collapse animation with showing overflowed content? I guess, I can imagine some crazy case, but it would be awesome if you can give some real example.

Maybe it is better to wrap your scrollable content into a scrollable fixed-height div instead of updating react-collapse?

[Jonathan-Millar]

I don't think its that crazy an idea. I am using it for a side menu that has children links. There could be hundreds so having a fixed height keeps the side menu usable, and scrolling within the container ensures I can still access all list elements. You are correct, adding a scrollable container inside would work, but why enforce overflow:hidden? I think its a great default but let dev's override it I think

[nkbt]

It is quite common to have overflow style for different elements, so it is very likely I'll get another issue with "Collapse does not work" title just because overflow is set to visible or something. overflow:hidden here is part of component's internal implementation, it is necessary to operate as intended.

If something can be achieve as easy as having internal div with overflow:auto, then isn't it better then updating lib itself?

We can add this to Gotchas section in readme instead (there might be lots of edge cases, by the way).

PS: still discussing though, I'm not quite sure if I am right

[Jonathan-Millar]

I think you should leave overflow:hidden so that when 90% of people use it,
it works as you designed it. Move its application as I proposed so it can
be over ridden. I doubt this will result in any other issues being opened
as they would have to purposefully over ride it to get different behaviour
then you originally intended.

On Tue, Nov 17, 2015, 1:42 AM Nik Butenko [email protected] wrote:

It is quite common to have overflow style for different elements, so it
is very likely I'll get another issue with "Collapse does not work" title
just because overflow is set to visible or something. overflow:hidden
here is part of component's internal implementation, it is necessary to
operate as intended.

If something can be achieve as easy as having internal div with
overflow:auto, then isn't it better then updating lib itself?

We can add this to Gotchas section in readme instead (there might be lots
of edge cases, by the way).

PS: still discussing though, I'm not quite sure if I am right

—
Reply to this email directly or view it on GitHub
#16 (comment).

[nkbt]

After some more thinking I will merge this in. As a developer I would prefer to have more freedom. I need to ask you to add couple of lines to the readme about the feature and warn to not specify height and overflow if you want collapse to work as intended.

Thank you, @Jonathan-Millar

[nkbt]

Published 1.5.0 https://github.com/nkbt/react-collapse/releases/tag/v1.5.0