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

Fix/unmounting select #512

Merged
merged 5 commits into from
Dec 12, 2018
Merged

Fix/unmounting select #512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
84e82db
fix(select): only destroy foundation if it exists
hvolschenk Dec 11, 2018
027ebd7
fix(checkbox): only destroy foundation if it exists
hvolschenk Dec 11, 2018
fe7955d
fix(radio): only destroy foundation if it exists
hvolschenk Dec 11, 2018
cb99f10
fix(text-field): only destroy foundation if it exists
hvolschenk Dec 11, 2018
1088ea5
fix(text-field): only destroy foundation if it exists
hvolschenk Dec 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/checkbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ export class Checkbox extends Component {
}

componentWillUnmount() {
this.foundation_.destroy();
if (this.foundation_) {
this.foundation_.destroy();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change (and all the others like it below) are not testable.
This means the PR here will bring the coverage amount down substantially.

It's a bit of a chicken/egg scenario as I need the foundation to not exist to check that destroy() is not called, but I need the foundation to exist to stub out destroy(). Sadface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merging #512 into master will increase coverage by 0.01%

Huh? If you say so codecov.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha - ya that makes sense. I believe all components have a foo calls destory on unmount test so we should be fine. Thanks for opening PR!

}

init = (el) => {
Expand Down
4 changes: 3 additions & 1 deletion packages/radio/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ class Radio extends React.Component {
}

componentWillUnmount() {
this.foundation_.destroy();
if (this.foundation_) {
this.foundation_.destroy();
}
}

componentDidUpdate(prevProps, prevState) {
Expand Down
4 changes: 3 additions & 1 deletion packages/select/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ export default class Select extends React.Component {
}

componentWillUnmount() {
this.foundation_.destroy();
if (this.foundation_) {
this.foundation_.destroy();
}
}

onChange = (evt) => {
Expand Down
4 changes: 3 additions & 1 deletion packages/text-field/helper-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ export default class HelperText extends React.Component {
}

componentWillUnmount() {
this.foundation_.destroy();
if (this.foundation_) {
this.foundation_.destroy();
}
}

get classes() {
Expand Down
4 changes: 3 additions & 1 deletion packages/text-field/icon/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ export default class Icon extends React.Component {
}

componentWillUnmount() {
this.foundation_.destroy();
if (this.foundation_) {
this.foundation_.destroy();
}
}

get adapter() {
Expand Down
4 changes: 3 additions & 1 deletion packages/text-field/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ class TextField extends React.Component {
}

componentWillUnmount() {
this.state.foundation.destroy();
if (this.state.foundation) {
this.state.foundation.destroy();
}
}

/**
Expand Down