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

London-10_Anu Thapaliya_cyf-hotel-react #605

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

Conversation

anuthapaliy
Copy link

No description provided.

Copy link

@Emiliecg Emiliecg left a 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" />

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?

return (
<div className="card-deck">
<div className="card">
<img src="Glasgow image.avif" className="card-img-top" alt="Glasgow" />

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?

Comment on lines 13 to 19
<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>

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"];

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;

Choose a reason for hiding this comment

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

Nice use of destructuring 😎

@sonarcloud
Copy link

sonarcloud bot commented Jul 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@migmow migmow added the reviewed A volunteer has reviewed this PR label Jul 10, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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

Successfully merging this pull request may close these issues.

3 participants