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

- [x] Header height is set in 1 place (for the links) #5322

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

Conversation

MichaelK-jpg
Copy link

  • Content is vertically centered (for any header height)

  • CSS is used to show all letters in Uppercase (don't type them in HTML)

  • Logo is an image wrapped with a link

  • CSS Variable is used for a blue color

  • Pseudo-element is used for a blue line below the active link

  • Code follows all the Code Style Rules ❗️

  • DEMO LINK

  • TEST REPORT LINK

  • Trochę nie wiem jak to zrobić lepiej, potrzebuję jakiś feedback :(

- [x] Content is vertically centered (for any header height)
- [x] CSS is used to show all letters in Uppercase (don't type them in HTML)
- [x] Logo is an image wrapped with a link
- [x] **CSS Variable** is used for a blue color
- [x] Pseudo-element is used for a blue line below the active link
- [x] Code follows all the [Code Style Rules ❗️](./checklist.md)
Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

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

Hey, you broke several code style rules. Please go through the checklist again and ensure all rules are applied: Checklist. Also, make sure to adhere to the requirements listed in the README.md. After applying all the fixes, try running the tests again. If the tests still don't pass, we will investigate the issue further.

src/index.html Outdated
Comment on lines 70 to 77
<div id="1"></div>
<div id="2"></div>
<div id="3"></div>
<div id="4"></div>
<div id="5"></div>
<div id="6"></div>
<div id="7"></div>
<div id="8"></div>
Copy link

Choose a reason for hiding this comment

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

What for is that?

src/index.html Outdated
Comment on lines 27 to 29
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap"
rel="stylesheet"
/>
Copy link

Choose a reason for hiding this comment

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

You don't need to import all the weights. Import just those that are actually used.

src/index.html Outdated
Comment on lines 63 to 66
<li><a href="#5">gadgets</a></li>
<li><a href="#6">tablets</a></li>
<li><a href="#7">photo</a></li>
<li><a href="#8">video</a></li>
Copy link

Choose a reason for hiding this comment

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

Fix formatting
image

Add empty lines between multiline sibling blocks of HTML
image

src/index.html Outdated
<div class="imgContainer">
<a href="#top">
<img
class="img"
Copy link

Choose a reason for hiding this comment

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

Class names represent the meaning of the content (not the styles or tag names)
good example
image

src/style.css Outdated
body {
margin: 0;
}

header {
Copy link

Choose a reason for hiding this comment

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

Don't use tag names for styling (except html and body)

src/style.css Outdated
}

.imgContainer {
margin: 10px 0 10px 50px;
Copy link

Choose a reason for hiding this comment

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

Be consistent with your vertical margins (Add only top or only bottom margin, but not both)

Comment on lines +46 to +47
text-decoration: underline 4px;
text-underline-offset: 1.2rem;
Copy link

Choose a reason for hiding this comment

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

There is requirement for that underline in mockup: -Use the ::after and position it relative to a link with is-active class

src/style.css Outdated
li {
list-style: none;
height: 15px;
margin-left: 13px;
Copy link

Choose a reason for hiding this comment

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

There should not be margins before the first and after the last list items

src/style.css Outdated

li {
list-style: none;
height: 15px;
Copy link

Choose a reason for hiding this comment

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

The height property is unnecessary.

@MichaelK-jpg
Copy link
Author

I am really struggling to pass the tests. The layout looks good in browser. But all tests are failed and its really hard to read them for me.

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

styles are not working on your demo.
image

tests are showing you the differences between what tests are expecting and how your layout looks. e.g in first test you can see that entire header should have margins on both sides but yours only has margin on the left. This happens because you're applying margin only to the logo not the header
image

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.

3 participants