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

London 10 - Elena Barker - React Module - CYF-Hotel-React #602

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

Conversation

ElenaBarker
Copy link

@ElenaBarker ElenaBarker commented Jun 21, 2023

@ElenaBarker ElenaBarker added to-review Student homework to be reviewed Ready for Review Week 1 Week 1 work is ready for review! Ready for Review Week 2 Week 2 work is ready for review! review requested and removed to-review Student homework to be reviewed labels Jul 11, 2023
@ElenaBarker ElenaBarker added the Ready for review week 3 Week 3 work is ready for review! label Jul 12, 2023
src/Glasgow.png Outdated

Choose a reason for hiding this comment

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

This is CSS feedback. Make all images same size in the CSS.

src/Footer.js Outdated

Choose a reason for hiding this comment

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

CSS feedback. You could change the text to display in a row instead. In App.css create something like .footer{ display: flex; flex-direction: row; justify-content: space-evenly; }

Choose a reason for hiding this comment

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

CSS feedback. You could change it into a row instead using css. Same solution as the code example I gave in Footer

src/App.js Outdated

Choose a reason for hiding this comment

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

Overall, the code demonstrates a good understanding of React components and their usage. However, there are areas for improvement, such as error handling, code formatting, and consistent naming conventions. Adding comments and documentation would also enhance the code's readability and maintainability.
To think about:
For your CustomerProfile you need a show/hide to handle profiles buttons.
The code could benefit from better code formatting and indentation for improved readability.
Error handling is minimal, and error messages are not displayed to the user, which can make debugging and user experience more challenging.
Some components, like Bookings and Restaurant, are missing proper opening and closing tags that can result in invalid JSX syntax.
The code could benefit from additional comments or documentation to provide more context and clarity, especially for complex logic and fetch requests.
What is great:
The code follows a modular structure with separate files for each component, which promotes code organization and reusability.
Components are imported and used correctly in the App component, making the code easier to read and maintain.
The usage of props in components like Footer and CustomerProfile allows for customization and flexibility.
The Search component handles user input and searching functionality effectively.
The SearchResults component displays search results and provides interactive features like highlighting selected rows and showing customer profiles.
The TouristInfoCard component showcases information about different tourist destinations in a visually appealing way.

Copy link

@sherif98 sherif98 left a comment

Choose a reason for hiding this comment

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

Thanks for your work Elena, I left some comments. Let me know if you have any questions.

src/Bookings.js Outdated Show resolved Hide resolved
src/Bookings.js Outdated Show resolved Hide resolved
src/CustomerProfile.js Outdated Show resolved Hide resolved
src/CustomerProfile.js Outdated Show resolved Hide resolved
Comment on lines 15 to 18
function highlightSelectedRow(id) {
setIsActive(!isActive);
setSelectedId(id);
}

Choose a reason for hiding this comment

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

I think you're mixing the logic of highlighting the row when it is clicked (i.e. task 15) and clicking on the Show profile button (i.e. tasks 20 and 21).

Clicking on the row should highlight it without making any changes to the customer profile. Additionally, the user should be able to highlight multiple rows at the same time.

Trying clicking on different multiple rows in https://strong-meerkat-359514.netlify.app/ to experience how it should look like. Let me know if you can't spot the differences and I'll try to elaborate more.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @sherif98 , may I ask for help with this issue, please?

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

sonarcloud bot commented Aug 27, 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
Ready for Review Week 1 Week 1 work is ready for review! Ready for Review Week 2 Week 2 work is ready for review! Ready for review week 3 Week 3 work is ready for review! review requested reviewed A volunteer has reviewed this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants