Skip to content
This repository has been archived by the owner on Feb 9, 2024. It is now read-only.

London-9-Turing-Farnoosh_Moayeri-React-WEEK1 #549

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

Farnooshmo
Copy link

@Farnooshmo Farnooshmo commented Mar 3, 2023

**CYF Hotel**

@Farnooshmo
Copy link
Author

@qingwei91 Hi, could you please review my code?

src/App.js Outdated
return (
<div className="App">
<header className="App-header">CYF Hotel</header>
<Heading />
<TouristInfoCards

Choose a reason for hiding this comment

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

small improvement: you can store these data in a data structure like a list of object, and then produce List of TouristInfoCards from that.

This approach will be more flexible, and less verbose

src/Bookings.js Outdated
console.info("TO DO!", searchVal);
const filteredBooking = bookings.filter((name) =>
name.firstName.toLowerCase() === searchVal.toLowerCase() ||
name.surname.toLowerCase() === searchVal.toLowerCase() ||

Choose a reason for hiding this comment

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

I think the top 2 conidition might be redundant since you're performing includes search next?

Copy link
Author

Choose a reason for hiding this comment

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

You are right, they can be deleted.

src/Bookings.js Outdated
name.surname.toLowerCase() === searchVal.toLowerCase() ||
name.firstName.toLowerCase().includes(searchVal.toLowerCase()) ||
name.surname.toLowerCase().includes(searchVal.toLowerCase())
? setBookings([name])

Choose a reason for hiding this comment

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

If you're performing some state changes, it might be better to call setBookings outside of filter function.

Generally, filter function should be pure, pure in this context means it does not mutate any variables that is outside of the scope of the function.

@Farnooshmo
Copy link
Author

@qingwei91 Thank you for the review; I fixed them, could you please review them?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants