-
-
Notifications
You must be signed in to change notification settings - Fork 767
London-10 | Iryna Lypnyk | React | cyf-hotel-react #608
base: master
Are you sure you want to change the base?
Conversation
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a lot to comment on all looks good a couple of styling comments
if(searchVal) { | ||
const filteredBookings = bookings.filter(booking => { | ||
const {firstName, surname} = booking; | ||
const preparedSearchVal = searchVal.toLowerCase(); | ||
const preparedFirstName = firstName.toLowerCase(); | ||
const preparedSurname = surname.toLowerCase(); | ||
return preparedFirstName.includes(preparedSearchVal) || preparedSurname.includes(preparedSearchVal) | ||
}); | ||
setFilteredBookings(filteredBookings); | ||
} else { | ||
setFilteredBookings(bookings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done, everything that follows has nothing to do with functionality as they are the exact same but want to talk about coding style. Do take these with a pinch of salt as styling varies from developer to developer, but worth sharing as it's always nice to know someone else's style.
I, personally, find that having too many variables can sometimes make the code harder to follow. So for example there are quite a few temporary variables within your filter that are reassigned. Sometimes that makes it easier to read but sometimes it can make it harder and is a balancing act to be aware of.
The second comment is that as your else branch is the case where you do nothing to the original bookings, it can be done in a different way to having two different calls to the setter.
const search = (searchVal) => {
let searchQuery = searchVal.toLowerCase();
let bookingResults = bookings;
if (searchQuery) {
bookingResults = bookings.filter({ firstName, surname } => {
return firstName.toLowerCase().includes(searchQuery) || surname.toLowerCase().includes(searchQuery);
});
}
setFilteredBookings(bookingResults)
}
/> | ||
<button className="btn btn-primary">Search</button> | ||
<SearchButton/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a space
<SearchButton />
const SearchResults = (props) => { | ||
const { results } = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do this in place:
const SearchResults = ({ results }) => {...}
}; | ||
|
||
|
||
return ( | ||
<div className="App-content"> | ||
<div className="container"> | ||
<Search search={search} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think how you'd modify this code so that the search function lives in the search component?
Some questions that might help you think about a way you could are:
- Is the bookings state needed anywhere else other than the search?
- Which setState functions would you need to pass down into the search component
No description provided.