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 #4919

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

add task solution #4919

wants to merge 6 commits into from

Conversation

roksolanap
Copy link

  • 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

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

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

Add links to the demo and tests report to PR description (19 here)

Try to pass all tests, feel free to ask for help in the chat if you need

--nav-backgroundcolor: #fff;
}

* {

Choose a reason for hiding this comment

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

better not adding a lot of styles to * selector without necessary

For example, if you add font-family and font-size to body they will be inherited by all nested elements
It is not a good idea to reset the margin for all elements, better to do it for the list of tags you really need

What really should be here is box-sizing property

src/style.css Outdated Show resolved Hide resolved
src/style.css Outdated
background-color: var(--nav-active);
border-radius: 8px;
height: 4px;
top: 34px;

Choose a reason for hiding this comment

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

bottom: 0 instead of "magic" 34px top

src/style.css Outdated
width: 40px;
}

.nav li {

Choose a reason for hiding this comment

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

do not add styles using combined selectors without necessary
just add class for all li elements and use this class to add styles

fix all similar cases

src/style.css Outdated
Comment on lines 61 to 67
.nav li .is-active {
color: var(--nav-active);
}

.nav li a:hover {
color: var(--nav-active);
}

Choose a reason for hiding this comment

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

you can list selectors with same styles separated by comma:

.example-selector-1,
.example-selector-2,
.example-selector-3:hover {
  // some styles
}

@roksolanap roksolanap requested a review from TarasHoliuk May 4, 2024 17:26
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.

Hi, there is no demo link, also you need to add the demo link to the Pull Request description
image

src/index.html Outdated

<nav class="nav">
<ul class="nav__list">
<li class="nav_li">

Choose a reason for hiding this comment

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

fix class everywhere

Suggested change
<li class="nav_li">
<li class="nav__item">

src/index.html Outdated
<ul class="nav__list">
<li class="nav_li">
<a
class="is-active nav_a"

Choose a reason for hiding this comment

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

The same as above

Suggested change
class="is-active nav_a"
class="is-active nav__link"

src/style.css Outdated
display: flex;
align-items: center;
justify-content: space-between;
background-color: var(--header-color);

Choose a reason for hiding this comment

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

This color is default

Suggested change
background-color: var(--header-color);

@roksolanap
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

@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.

you forgot to provide these two important links in the PR description
image

src/style.css Outdated
Comment on lines 57 to 63
.nav__link:hover {
color: var(--nav-active);
}

.is-active {
color: var(--nav-active);
}

Choose a reason for hiding this comment

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

Suggested change
.nav__link:hover {
color: var(--nav-active);
}
.is-active {
color: var(--nav-active);
}
.nav__link:hover,
.is-active {
color: var(--nav-active);
}

Copy link
Author

Choose a reason for hiding this comment

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

done, thank you

@roksolanap
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 ❗️

@roksolanap roksolanap requested a review from etojeDenys May 6, 2024 07:32
Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

You've got this, keep pushing forward!

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