-
-
Notifications
You must be signed in to change notification settings - Fork 768
London-10_Anu Thapaliya_cyf-hotel-react #605
base: master
Are you sure you want to change the base?
Conversation
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.
Great start! I've added a few suggestions for improvements.
Also, could you add a Netlify link if you have one?
src/Heading.js
Outdated
const Heading = () => { | ||
return ( | ||
<header> | ||
<img src="https://image.flaticon.com/icons/svg/139/139899.svg" alt ="Hotel Logo" /> |
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.
There is no logo showing on the page. I think it is because this link is not a valid image. Maybe you can find a different image yourself and change the link?
src/TouristInfoCards.js
Outdated
return ( | ||
<div className="card-deck"> | ||
<div className="card"> | ||
<img src="Glasgow image.avif" className="card-img-top" alt="Glasgow" /> |
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.
I don't see any images for the tourist info cards when I run the app locally. Where are these coming from? For example, what is Glasgow image.avif
referring to?
src/TouristInfoCards.js
Outdated
<div className="card"> | ||
<img src="Manchester image.webp" className="card-img-top" alt="Manchester" /> | ||
<div className="card-body"> | ||
<h5 className="card-title">Manchester</h5> | ||
<p className="card-text">Visit the exciting city of Football.</p> | ||
<a href="https://www.visitmanchester.com/" className="btn btn-primary">More Info</a> | ||
</div> |
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.
The code for these three cards looks very similar, with some differences in the content. Have you considered using a component to avoid duplicating code?
import "./App.css"; | ||
|
||
const App = () => { | ||
const addresses = ["123 Fake Street, London, E1 4UD", "[email protected]", "0123 456789"]; |
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.
This array looks like it contains data other than addresses. Can you think of a clearer name for the prop?
import React from "react"; | ||
|
||
const Footer = (props) => { | ||
const { addresses } = 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.
Nice use of destructuring 😎
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
No description provided.