Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/AppHeader/Bakhat #35

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Feature/AppHeader/Bakhat #35

merged 3 commits into from
Feb 29, 2024

Conversation

BakhatBegum
Copy link
Collaborator

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

I added header component.

Questions

@BakhatBegum
Copy link
Collaborator Author

I added header component in AppHeader.jsx

// what goes in here? there is no content in the AppHeader component
// );
// export default AppHeader;
import Logo from "@/assets/spa-logo.png";
Copy link
Collaborator

@PERicci PERicci Feb 28, 2024

Choose a reason for hiding this comment

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

I believe that the correct file is hotel-logo.png rather than spa-logo.png, as we can see in the last step of the issue: "render the hotel's logo in an <img>".

Copy link
Collaborator

@PERicci PERicci left a comment

Choose a reason for hiding this comment

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

Logo in the may be is incorrect.

@BakhatBegum BakhatBegum mentioned this pull request Feb 28, 2024
4 tasks
HadikaMalik added a commit that referenced this pull request Feb 28, 2024
…chResults

Component/SearchRestults Implementation #35
@BakhatBegum
Copy link
Collaborator Author

BakhatBegum commented Feb 29, 2024

Thank you Pedro I did change the logo.

// what goes in here? there is no content in the AppHeader component
// );
// export default AppHeader;
import Logo from "@/assets/hotel-logo.png";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect. Thanks.

Copy link
Collaborator

@PERicci PERicci left a comment

Choose a reason for hiding this comment

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

The AppHeader component implementation matches with the issue's requirement.
Thanks.

<header className="app__header">
<h1 className="app__heading">CYF Hotel</h1>
</header>
<AppHeader/>
Copy link

Choose a reason for hiding this comment

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

Well done for this, you have successfully extracted appheader completing step1 +2 of the instructions

const AppHeader = () => (
<header className="app__header">
<h1 className="app__heading">CYF Hotel</h1>
<img src={Logo} alt="main image" />
Copy link

Choose a reason for hiding this comment

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

This alt text is not descriptive at all.
When writing alt text, try to imagine a blind person listening to it. Does it accurately descibe the image ?

// what goes in here? there is no content in the AppHeader component
// );
// export default AppHeader;
import Logo from "@/assets/hotel-logo.png";
Copy link

Choose a reason for hiding this comment

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

good use of the @ symbol here for imports

return true;
});

test('renders AppHeader component', () => {
Copy link

Choose a reason for hiding this comment

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

using test here is fine, but it looks inconsistent using "test" in one section and "it" in another. It is always best to be consistent with your code

@eliza-an
Copy link

eliza-an commented Mar 9, 2024

It is also best to keep your PRs consistent without saying who did what specifically as this is a collaborative project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants