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

Develop #5374

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

Develop #5374

wants to merge 4 commits into from

Conversation

romka12254
Copy link

DEMO LINK
TEST REPORT LINK
❗️ Copy this Checklist to the Pull Request description after links, and put - [x] before each point after you checked it.

Header height is set in 1 place (for the links)
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 ❗️

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Hey there! It looks like there's a small hiccup in your anchor links – they shouldn't contain spaces. Make sure your href attributes are correctly formatted, either with a # for same-page IDs or a valid URL for other pages. This will ensure your page works perfectly for your users! Keep up the great work, and with this quick fix, your code will be shipshape in no time! ⚓🛠️

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/index.html Outdated
Comment on lines 28 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"

Choose a reason for hiding this comment

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

The href attribute in the link tag for Google Fonts is too long. It should be split into multiple lines to adhere to the 80 character limit per line. You could place each font weight on a new line to improve readability.

src/index.html Outdated
<ul class="header__list">
<li class="header__item">
<a
href="#!"

Choose a reason for hiding this comment

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

Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they link to an ID within the same page, or they should be a valid URL if linking to other pages.

src/index.html Outdated
</li>
<li class="header__item">
<a
href="#!"

Choose a reason for hiding this comment

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

Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they link to an ID within the same page, or they should be a valid URL if linking to other pages.

src/index.html Outdated
</li>
<li class="header__item">
<a
href="#!"

Choose a reason for hiding this comment

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

Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they link to an ID within the same page, or they should be a valid URL if linking to other pages.

src/index.html Outdated
</li>
<li class="header__item">
<a
href="#!"

Choose a reason for hiding this comment

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

Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they link to an ID within the same page, or they should be a valid URL if linking to other pages.

src/index.html Outdated
</li>
<li class="header__item">
<a
href="#!"

Choose a reason for hiding this comment

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

Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they link to an ID within the same page, or they should be a valid URL if linking to other pages.

src/index.html Outdated
</li>
<li class="header__item">
<a
href="#!"

Choose a reason for hiding this comment

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

Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they link to an ID within the same page, or they should be a valid URL if linking to other pages.

src/index.html Outdated
</li>
<li class="header__item">
<a
href="#!"

Choose a reason for hiding this comment

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

Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they link to an ID within the same page, or they should be a valid URL if linking to other pages.

src/style.css Outdated
Comment on lines 42 to 44
li {
margin-right: 20px;
}

Choose a reason for hiding this comment

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

Avoid using tag names for styling (except html and body). Instead, style elements using .class. For example, consider adding a class to the li elements within .header__list and style that class instead.

src/style.css Outdated
}

html {
font-family: Roboto, Arial, sans-serif;

Choose a reason for hiding this comment

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

Remember to use fallback fonts - alternative font-family in case the main one doesn't work. For example, font-family: Roboto, Arial, sans-serif; is good, but you could add more alternatives like font-family: Roboto, 'Helvetica Neue', Arial, sans-serif;.

@romka12254 romka12254 marked this pull request as draft September 16, 2024 20:42
@romka12254 romka12254 marked this pull request as ready for review September 16, 2024 20:43
@romka12254 romka12254 closed this Sep 16, 2024
@romka12254 romka12254 reopened this Sep 16, 2024
Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Good job! 👍
Let's make your code better

src/index.html Outdated
Comment on lines 45 to 50
<nav class="header__nav">
<ul class="header__list">
<li class="header__item">
<a
href="#"
class="header__link is-active"

Choose a reason for hiding this comment

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

Fix classes

Suggested change
<nav class="header__nav">
<ul class="header__list">
<li class="header__item">
<a
href="#"
class="header__link is-active"
<nav class="nav">
<ul class="nav__list">
<li class="nav__item">
<a
href="#"
class="nav__link is-active"

src/index.html Outdated
Comment on lines 54 to 55
</li>
<li class="header__item">

Choose a reason for hiding this comment

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

Add the blank line between the links

Suggested change
</li>
<li class="header__item">
</li>
<li class="header__item">

src/style.css Outdated
@@ -1,3 +1,81 @@
body {
margin: 0;
padding: 0;

Choose a reason for hiding this comment

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

The body hasn't a padding

Suggested change
padding: 0;

src/style.css Outdated
font-size: 12px;
font-weight: 500;

--active-color: #00acdc;

Choose a reason for hiding this comment

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

Create a variable with :root selector

src/style.css Outdated
Comment on lines 18 to 19
justify-content: space-between;
}

Choose a reason for hiding this comment

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

Suggested change
justify-content: space-between;
}
justify-content: space-between;
padding: 0 50px;
box-sizing: border-box;
}

src/style.css Outdated
Comment on lines 21 to 23
.header__nav {
margin: 0 50px 0 0;
}

Choose a reason for hiding this comment

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

Suggested change
.header__nav {
margin: 0 50px 0 0;
}

src/style.css Outdated
display: block;
width: 40px;
height: 40px;
margin: 10px 0 10px 50px;

Choose a reason for hiding this comment

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

Suggested change
margin: 10px 0 10px 50px;

src/style.css Outdated
margin: 0;
}

a:hover {

Choose a reason for hiding this comment

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

Use only class selector

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Well done! 🔥

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