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

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

Conversation

OTsiolkovskiy
Copy link

  • DEMO LINK

  • TEST REPORT 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 ❗️

/>
</a>
</div>
<div>

Choose a reason for hiding this comment

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

Please add empty lines between multiline sibling elements - here, in <ul> and in <head>.

Copy link
Author

Choose a reason for hiding this comment

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

fixed, added empty lines

src/index.html Outdated
</div>
<div>
<nav class="nav">
<ul class="nav_list">

Choose a reason for hiding this comment

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

In terms of BEM, nav is a block and list, item are elements. According to BEM syntax, block and element names should be separated with double underscore in the class name - and in your code they're separated with single underscore. Please correct that.

Copy link
Author

Choose a reason for hiding this comment

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

fixed, added BEM syntax

src/style.css Outdated
text-decoration: solid 2px black;
}

.nav__link:hover {

Choose a reason for hiding this comment

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

.nav__link is not used in your HTML, so this style can be removed

Copy link
Author

Choose a reason for hiding this comment

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

removed

src/style.css Outdated
text-decoration: none;
color: #000;
display: inline-block;
height: 60px;

Choose a reason for hiding this comment

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

Both height and line-height are not needed here. With line-height you set the height of text to 60px, so if the parent item contains single line of text and has no vertical margins/paddings/borders, it will already be 60px high. So height is redundant here.

Copy link
Author

Choose a reason for hiding this comment

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

removed

Copy link

@undeletable undeletable left a comment

Choose a reason for hiding this comment

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

The solution looks nice. It just needs some improvements, mostly related to guidelines. Please check the comments.

Copy link

@alexander-ignatow alexander-ignatow left a comment

Choose a reason for hiding this comment

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

For now, let's go with it, however, there are two issues, that are to be fixed in the next tasks:

  1. Make sure to submit only the files, related to the task. For instance dist folder definitely should not be here
  2. See my comment for BEM blocks below

Otherwise, the task looks good 👍

}

.logo {
margin-left: 50px;

Choose a reason for hiding this comment

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

The block shouldn't influence its environment, meaning you shouldn't set the external geometry (margin) or positioning for the block.

One of the techniques to avoid such problem is using mixes
https://en.bem.info/methodology/css/#external-geometry-and-positioning

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