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

Feature/select g house #34

Merged
merged 5 commits into from
Jun 6, 2017
Merged

Feature/select g house #34

merged 5 commits into from
Jun 6, 2017

Conversation

philawsophizing
Copy link
Contributor

@philawsophizing philawsophizing commented Jun 4, 2017

ref #22

{props.db.map((guestHouses) => {
const boundItemClick = handleClick.bind(this, guestHouses.name);
return (
<button
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this button should be a different component and then the bind function wont be inside the map function

currentGuestHouse(guestHouse) {
const current = this.state.db.filter((curr) => {
if (curr.name === guestHouse) {
return curr;
Copy link
Contributor

Choose a reason for hiding this comment

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

curr and current?

@philawsophizing
Copy link
Contributor Author

@esraajb more like this? 🍁

Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

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

looks good 👍, left a couple of comments. Also, no issue referred



class SelectGuesthouse extends Component {
constructor(props) {
super(props);
this.state = {
db: {},
title: 'PAS',
tagLine: 'Park & Sleep',
Copy link
Contributor

Choose a reason for hiding this comment

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

are title and tagLine ever change? if not, then its pointless to put them in a state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are going to be changed (header will update based on these values). Next PR.

db: {},
title: 'PAS',
tagLine: 'Park & Sleep',
db: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe instead of db, use 'guestHousesList'? might be clearer for the reader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout

@philawsophizing
Copy link
Contributor Author

philawsophizing commented Jun 6, 2017

@esraajb ref #22 was in the commit messages 😄 should it be in the comment as well?

Changes made.

@jbesraa
Copy link
Contributor

jbesraa commented Jun 6, 2017

Yea, should be in the description

@philawsophizing
Copy link
Contributor Author

@esraajb Cool cool, done. And will do going forward. Good to merge?

@jbesraa jbesraa merged commit 9a0c54f into master Jun 6, 2017
@jbesraa jbesraa deleted the Feature/SelectGHouse branch June 6, 2017 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants