search

nkbt / react-height

Component-wrapper to determine and report children elements height
MIT License
180 stars 27 forks source link

getElementHeight Not Null Checking #25

Closed ambroselittle closed 7 years ago

ambroselittle commented 7 years ago

I just started working on this big code base that, among many other things, uses this library. I think it may be react-collapse that is using it which we are using...

The point is, my case my be very obscure and not worth fixing. That said..

I was trying to get Jest working to test new components that use old components that ultimately use this. And in that context, I got "cannot read property 'clientHeight' of undefined" in the mix. Turns out that somehow a null is being passed to this getElementHeight method.

My quick/naive solution: just do a ternary null check: return el ? el.clientHeight : 0;

It solved my problem anyways, though I suspect it is obscuring a larger issue. In my case, I just wanted rendering/snapshotting to work in test, even if it's "not right" in terms of something you'd really want to see rendering.

Still, seems like not a bad defensive thing to do here, so I thought I'd mention it.

nkbt commented 7 years ago

Thanks for heads up!

Checking if element is there would make sense only for tests, and my usual rule is to never write code that would be necessary only for tests.

Now regarding react-collapse. There is v3 of it and it is no longer using react-height, so this project is no longer under active development anymore. More like in the relaxed support mode

Cheers, Nik