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

London10 | Adrian Ilovan | cyf-hotel-react #589

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

Conversation

AdrianIlovan
Copy link

No description provided.

@AdrianIlovan AdrianIlovan changed the title London10 | Adrian Ilovan | cyf-hotel-react | Week 1 London10 | Adrian Ilovan | cyf-hotel-react Jun 23, 2023
return(
<header className="App-header">
<img alt="App-logo" className="App-logo" src="https://marketplace.canva.com/EAE8Xyb2xY8/1/0/1600w/canva-gold-exclusive-royal-luxury-hotel-logo-izeElzvImEY.jpg"></img>
<h1>CYF Hotel</h1></header>

Choose a reason for hiding this comment

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

It's a minor point, but just keep an eye on the formatting of your embedded html.
It would be good </header> was on a new line

import React from "react";


const SearchButton = () => {

Choose a reason for hiding this comment

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

Nice 👍

@@ -0,0 +1,31 @@
import React from "react";

Choose a reason for hiding this comment

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

This looks great. But it seems like there's some duplication here - the structure of each card is the same, and we are repeating that structure 3 times.
Using React, can you think of how you might be able to reduce that duplication? 😄

Copy link
Author

Choose a reason for hiding this comment

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

Yes I think I can create a separate component "const Card" and pass all object info in there so I can add any other card with ease . thanks for letting me know :)


const Footer = () => {

const myProps = ["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.

It sounds like this data should be passed into the component using React props. You can have a look at how props normally work here: https://www.w3schools.com/react/react_props.asp
Can you think of how to update the code so the data is defined in the parent component, and is passed to the Footer component as a prop?

Copy link
Author

Choose a reason for hiding this comment

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

I will make the updates , thanks :)

import FakeBookings from "./data/fakeBookings.json";
import moment from "moment/moment";

const SearchResults = (props) => {

Choose a reason for hiding this comment

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

This looks great!
You did a good job of passing the props in and using them in the component 👍

<td>{booking.roomId}</td>
<td>{booking.checkInDate}</td>
<td>{booking.checkOutDate}</td>
<td>{moment(booking.checkOutDate).diff(booking.checkInDate, "days")}</td>

Choose a reason for hiding this comment

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

Nice 👏

const SearchResults = (props) => {
const [selectedRows, setSelectedRows] = useState([]);
const selectedClicks = (bookingId) => {
setSelectedRows((selectedClickedRows) => {

Choose a reason for hiding this comment

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

Awesome - great work! 😄

@moneyinthesky
Copy link

Great work so far - good luck with the rest of the project 😄

@sonarcloud
Copy link

sonarcloud bot commented Jul 7, 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 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@migmow migmow added the reviewed A volunteer has reviewed this PR label Jul 10, 2023
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