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.

Sign up for GitHub

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

Jump to bottom

Accordion needs a height calculation on mount #816

Merged
merged 4 commits into from
May 10, 2018
Merged

Accordion needs a height calculation on mount #816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3ce8be
Accordion needs a height calculation on mount
snide May 10, 2018
7bf1e66
remove copy/pasta ally code
snide May 10, 2018
7465907
changelog
snide May 10, 2018
5f6ad21
add focus ring, fix styling in accordion
snide May 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

**Bug fixes**

- Fixes height calculation error on `EuiAccordion` when it starts loads in an open state. ([#816](https://github.com/elastic/eui/pull/816))
- Added aria-invalid labeling on `EuiFormRow` ([#777](https://github.com/elastic/eui/pull/799))
- Added aria-live labeling for `EuiToasts` ([#777](https://github.com/elastic/eui/pull/777))
- Added aria labeling requirements for `EuiBadge` , as well as a generic prop_type function `requiresAriaLabel` in `utils` to check for it. ([#777](https://github.com/elastic/eui/pull/777)) ([#802](https://github.com/elastic/eui/pull/802))
Expand Down
11 changes: 9 additions & 2 deletions src/components/accordion/_accordion.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,20 @@
text-align: left;
width: 100%;

&:hover, &:focus {
&:hover {
text-decoration: underline;
cursor: pointer;
}

&:focus {
background-color: $euiFocusBackgroundColor;
.euiAccordion__iconWrapper {
@include euiFocusRing;

color: $euiColorPrimary;
border-radius: $euiBorderRadius;
}
}

}

.euiAccordion__childWrapper {
Expand All @@ -24,6 +30,7 @@
;
}


$paddingSizes: (
xs: $euiSizeXS,
s: $euiSizeS,
Expand Down
11 changes: 0 additions & 11 deletions src/components/accordion/_accordion_form.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,6 @@
text-decoration: underline;
}
}

&:focus {
text-decoration: none;
background-color: transparent;

.euiAccordionForm__title {
text-decoration: underline;
background-color: $euiFocusBackgroundColor;
outline: solid $euiSizeXS / 2 $euiFocusBackgroundColor;
}
}
}
.euiAccordionForm {
border-top: $euiBorderThin;
Expand Down
17 changes: 13 additions & 4 deletions src/components/accordion/accordion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,19 @@ export class EuiAccordion extends Component {
this.onToggle = this.onToggle.bind(this);
}

componentDidUpdate() {
const height = this.state.isOpen ? this.childContent.clientHeight: 0;
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.

});
}

this.childWrapper.setAttribute('style', `height: ${height}px`);
componentDidMount() {
this.setChildContentHeight();
}

componentDidUpdate() {
this.setChildContentHeight();
}

onToggle() {
Expand Down Expand Up @@ -112,7 +121,7 @@ export class EuiAccordion extends Component {
className={buttonClasses}
>
<EuiFlexGroup gutterSize="s" alignItems="center" responsive={false}>
<EuiFlexItem grow={false}>
<EuiFlexItem grow={false} className="euiAccordion__iconWrapper">
{icon}
</EuiFlexItem>

Expand Down