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

add task solution #4691

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

add task solution #4691

wants to merge 12 commits into from

Conversation

Pjankusha
Copy link

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

if you have some problems with tests then feel free to ask for some help in the chat

src/index.html Outdated Show resolved Hide resolved
src/style.css Outdated Show resolved Hide resolved
@Pjankusha
Copy link
Author

I added changes, please review

@Pjankusha Pjankusha closed this Mar 8, 2024
@Pjankusha Pjankusha reopened this Mar 8, 2024
Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

your demo link doesn't work and all tests failed, don't hesitate to ask for some help in the chat

@Pjankusha
Copy link
Author

✅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

@andriy-shymkiv andriy-shymkiv left a comment

Choose a reason for hiding this comment

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

looking good, left a few suggestions

src/index.html Outdated
<nav class="nav">
<ul class="nav__list">
<li class="nav__item">
<a href="#" class="nav__link is-active blue-link">Apple</a>

Choose a reason for hiding this comment

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

I believe that you don't need this class

Suggested change
<a href="#" class="nav__link is-active blue-link">Apple</a>
<a href="#" class="nav__link is-active">Apple</a>

Choose a reason for hiding this comment

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

is-active is enough for such case

src/style.css Outdated
Comment on lines 7 to 10
width: 100%;
margin: 0;
padding:0;
font-family: Roboto, sans-serif;

Choose a reason for hiding this comment

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

Suggested change
width: 100%;
margin: 0;
padding:0;
font-family: Roboto, sans-serif;
margin: 0;
font-family: Roboto, sans-serif;

src/style.css Outdated
Comment on lines 37 to 46
.nav__link {
display: flex;
position: relative;
height: 60px;
justify-content: center;
align-items: center;
flex-direction: column;
text-decoration: none;
color: var(--main-text-color);
}

Choose a reason for hiding this comment

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

Suggested change
.nav__link {
display: flex;
position: relative;
height: 60px;
justify-content: center;
align-items: center;
flex-direction: column;
text-decoration: none;
color: var(--main-text-color);
}
.nav__link {
position: relative;
height: 60px;
text-decoration: none;
color: var(--main-text-color);
}

Copy link
Author

Choose a reason for hiding this comment

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

Hi, unfortunately your suggestion regarding removing flex in nav__link doesn't work.
Screenshot 2024-03-11 at 13 33 38
Screenshot 2024-03-11 at 13 33 10

Choose a reason for hiding this comment

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

You can try just to add text-align: center

Copy link
Author

Choose a reason for hiding this comment

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

Screenshot 2024-03-11 at 17 10 00 text-align: center doesn't work even if I change display to block. Regarding requirements to this task text should be centred, and height set to nav__links, flex in this case make it easy to do. And it is works right.

src/style.css Outdated
Comment on lines 48 to 54
.nav__item{
margin-left: 20px;
}

.nav__list > :first-child{
margin-left: 0;
}

Choose a reason for hiding this comment

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

you can just use not:first-child

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for suggestion. It is looks better.

src/style.css Outdated
Comment on lines 56 to 59
.blue-link,
.nav__link:hover {
color: var(--edit-text-color);
}

Choose a reason for hiding this comment

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

Suggested change
.blue-link,
.nav__link:hover {
color: var(--edit-text-color);
}
.is-active,
.nav__link:hover {
color: var(--edit-text-color);
}

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for this suggestion

andriy-shymkiv

This comment was marked as resolved.

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Make sure to deploy your page. Feel free to ask for help in the chat

Copy link

@Viktor-Kost Viktor-Kost left a comment

Choose a reason for hiding this comment

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

Please make sure you've redeployed your work, since at your DEMO we can still see previous solution.
If you need any help feel free to ask at fe_chat
Screenshot 2024-03-11 at 16 10 41

src/style.css Outdated
Comment on lines 6 to 9
html, body {
margin: 0;
font-family: Roboto, sans-serif;
}

Choose a reason for hiding this comment

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

html doesn't have default margin

Suggested change
html, body {
margin: 0;
font-family: Roboto, sans-serif;
}
html {
font-family: Roboto, sans-serif;
}
body {
margin: 0;
}

src/style.css Outdated
Comment on lines 37 to 46
.nav__link {
display: flex;
position: relative;
height: 60px;
justify-content: center;
align-items: center;
flex-direction: column;
text-decoration: none;
color: var(--main-text-color);
}

Choose a reason for hiding this comment

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

You can try just to add text-align: center

@Pjankusha Pjankusha closed this Mar 12, 2024
@Pjankusha Pjankusha reopened this Mar 12, 2024
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.

5 participants