Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accordion needs a height calculation on mount #816

Merged
merged 4 commits into from
May 10, 2018
Merged

Conversation

snide
Copy link
Contributor

@snide snide commented May 10, 2018

Fixes #805

Makes sure that the child content of an accordion gets a height if its set to be initially open.

image

cc @ppisljar and @timroes who ran into this issue in elastic/kibana#18579

@snide snide requested a review from chandlerprall May 10, 2018 20:39
@snide
Copy link
Contributor Author

snide commented May 10, 2018

Also took the time to make the focus state look a bit better.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@snide snide merged commit 3d76684 into elastic:master May 10, 2018
@snide snide deleted the bug/805 branch May 10, 2018 21:38
@snide snide mentioned this pull request May 11, 2018
setChildContentHeight = () => {
requestAnimationFrame(() => {
const height = this.state.isOpen ? this.childContent.clientHeight: 0;
this.childWrapper.setAttribute('style', `height: ${height}px`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason why we're using setAttribute here instead of setting the height to the state and set it to the style attribute in the render function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been awhile since I originally wrote this one. Usually I would have written this with state so I'm guessing I did it for a speed reason or the animation got hitched. I've found with animations like these sometimes state can be slow. For example, I need the animationFrame because it couldn't grab the height fast enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fair. I would btw recommend using el.style.height = ... instead of setAttribute, since it's a) working more stable across all browsers and b) would not destroy any potential other styles set on the same element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants