Skip to content

Fully rewritten the App with redux.#4

Open
bitstasis wants to merge 2 commits into
masterfrom
week4
Open

Fully rewritten the App with redux.#4
bitstasis wants to merge 2 commits into
masterfrom
week4

Conversation

@bitstasis
Copy link
Copy Markdown
Owner

No description provided.

@bitstasis bitstasis requested a review from pavlo-berezin June 1, 2020 03:52
@pavlo-berezin pavlo-berezin changed the base branch from master to week3 June 9, 2020 21:50
@pavlo-berezin pavlo-berezin changed the base branch from week3 to master June 9, 2020 21:50
})

const mapDispatchToProps = (dispatch) => ({
setLocation: (location) => dispatch(updateLocation(location))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you could just use react-router-redux https://www.npmjs.com/package/react-router-redux :)

}

const mapStateToProps = (state) => ({
location: state.nav.location
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

location from props is never used so you can safely remove it

<Card key={data_entry.title} title={data_entry.title} number={data_entry.number} ranking={data_entry.ranking} daily={data_entry.daily} />
)}
{Object.values(entry.covidinfo)
.filter(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't put so much logic inside of markup. Consider refactoring this piece

});

const mapDispatchToProps = (dispatch) => ({
setError: (value) => dispatch(updateError(value)),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

never used

) {
return countries;
}
return countries.filter(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just a suggestion:

const startsWithCountryName = (value) => value.toLowerCase().startsWith(countryName);

return countries.filter(({ country, iso2, iso3 }) =>
  startsWithCountryName(country) ||
  startsWithCountryName(iso2) ||
  startsWithCountryName(iso3)
);

});

}
this.props.setValue(event.target.value);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok for test project, but in your case it doesn't make a lot of sense to process input through redux.

@pavlo-berezin pavlo-berezin self-requested a review June 9, 2020 22:05
@pavlo-berezin pavlo-berezin removed their request for review December 20, 2021 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants