London10_Jan_Softa_cyf_hotel_react#615
London10_Jan_Softa_cyf_hotel_react#615softacoder wants to merge 17 commits intoCodeYourFuture:masterfrom
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
sherif98
left a comment
sherif98
left a comment
There was a problem hiding this comment.
Overall looks great Jan! I added a couple of comments.
| import Restaurant from "./Restaurant"; | ||
|
|
||
| const App = () => { | ||
| const contactDetails = [ |
There was a problem hiding this comment.
Can we move the contactDetails inside the Footer component? I don't think there is a need to define contactDetails here and pass it to the Footer component as props
| import React, { useEffect, useState } from "react"; | ||
| import ClipLoader from "react-spinners/ClipLoader"; | ||
| import Search from "./Search.js"; | ||
| // import SearchResults from "./SearchResults.js"; |
|
|
||
| useEffect(() => { | ||
| setIsLoading(true); | ||
| fetch(`https://cyf-react.glitch.me/customers/${props.id}`) |
There was a problem hiding this comment.
In Bookings.js you use async, await to do the fetch, while here you use .then() (i.e. promises chaining). I suggest using async, await here as well to make sure the code is consistent.
|
|
||
| if (!customerProfileData) { | ||
| return null; // or render a message indicating that the customer profile is not available | ||
| } |
There was a problem hiding this comment.
[no action] It's nice you're separating the different return statements and not putting them all into one big return.
| onChange={handleSearchInput} | ||
| /> | ||
| <button className="btn btn-primary">Search</button> | ||
| <SearchButton /> |
There was a problem hiding this comment.
I feel that putting the button into a separate SearchButton component is over-engineering. The component only has a single line to declare a button and doesn't maintain any state. What do you think of just inlining the here?
| ) : ( | ||
| <OutcomeSearch results={bookings} setCustomerId={setCustomerId} /> | ||
| )} | ||
| {customerId && <CustomerProfile id={customerId} />} |
There was a problem hiding this comment.
Is using the CustomerProfile component here needed? I see that you render the CustomerProfile component in the SearchResult.js line 54.
I think customerId is always undefined and thus it never renders the component here. Please take a look and let me know what are your thoughts on this. Thanks
3 participants
No description provided.