London-10 | Iryna Lypnyk | React | cyf-hotel-react #608
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
| 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.
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/> |
| const SearchResults = (props) => { | ||
| const { results } = props; |
There was a problem hiding this comment.
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.
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.