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

Nn 1452 location filter #204

Merged
merged 2 commits into from
Nov 29, 2018
Merged

Nn 1452 location filter #204

merged 2 commits into from
Nov 29, 2018

Conversation

lbennett-moj
Copy link
Contributor

No description provided.

event.preventDefault()
const { showFilters } = this.state
this.setState({
showFilters: !showFilters,
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the react docs you should use a different version of setState if you're referring to the current state.
this.setState((state) => { showFilters: !state.showFilters }

<div className="pure-u-md-1-4 padding-top padding-bottom-large padding-left">{genderSelect}</div>
<div>
<a className="clear-filters link clickable" {...linkOnClick(clearFilters)}>
Clear filters
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making the clear filters link a component? If you did that it would encapsulate the props that you're supplying using a function. Then there'd be no need to put a 'linkOnClick' function in utils.js.

Copy link
Contributor

@bell-pa bell-pa left a comment

Choose a reason for hiding this comment

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

Hi Lucy. See comments on PR for one suggested and one recommended (by React) change. I've approved the PR - so you can make the changes after merging if you prefer.

@lbennett-moj lbennett-moj merged commit ce346ab into master Nov 29, 2018
@bell-pa bell-pa deleted the NN_1452_location_filter branch January 16, 2019 14:54
@bell-pa bell-pa restored the NN_1452_location_filter branch January 16, 2019 14:55
@bell-pa bell-pa deleted the NN_1452_location_filter branch January 16, 2019 14:55
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.

2 participants