Fix: accordion observer issues#982
Conversation
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
code LGTM; will address further in #966 which refactors the mutation observer usage in EUI
| this.setChildContentHeight(); | ||
| } | ||
|
|
||
| componentWillUnmount() { |
There was a problem hiding this comment.
have you observed a case when the observer isn't disconnected? ref functions are [normally] called with null as the children components unmount, and the accordion's setChildContentRef is setup to respond accordingly
There was a problem hiding this comment.
Oh, that's a good point. I didn't think ref got recalculated on unmount. If that's true, then the componentWillUnmount hook isn't actually required. Is that also true with the new ref API in 16.3+?
There was a problem hiding this comment.
For the record, it does indeed call the ref function with null on unmount. I've closed that issue and removed that code from this PR.
I wasn't aware of that, and I'm still curious if the new ref API in 16.3 works the same way.
There was a problem hiding this comment.
16.3 works the same way, only changes to refs in that version is the addition of the forwardRef API. Thanks for testing!
|
Thanks for the PR! |
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
Needs Bug Fix added to changelog
|
@chandlerprall hopefully I did that right... |
chandlerprall
left a comment
chandlerprall
left a comment
There was a problem hiding this comment.
please move the changelog line into the **Bug fixes** section immediately below it
|
@w33ble requested a small placement change to the changelog, then this will be great to get in! |
|
@chandlerprall any chance you're planning to cut a bugfix release this week? |
w33ble
force-pushed
the
fix/accordion-observer-issues
branch
from
8aa3a03 to
2e6f498
Compare
don't attempt to read the clientHeight if it's no assigned
before trying to use setAttribute
w33ble
force-pushed
the
fix/accordion-observer-issues
branch
from
2e6f498 to
3f29698
Compare
|
@w33ble feel free to merge, I'll cut a release when this is in |
2 participants
Quick fix for #981
Mostly a quick patch to the Accordion so that the mutation observer stops causing uncaught errors.
setChildContentHeight, check thatthis.childContentis truthy before trying to read theclientHeightpropertysetChildContentHeight, check thatthis.childWrapperis set before trying to usesetAttribute