-
Notifications
You must be signed in to change notification settings - Fork 0
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
Css day 31 #35
base: registration-day-29
Are you sure you want to change the base?
Css day 31 #35
Conversation
</ul> | ||
<nav> | ||
<div className="NavBar"> | ||
<ul className="NavBar__ul"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't like class names that start with capital letter, navbar
or nav-bar
fits better here IMO.
There is no reason to add element name to its class name. I would go with NavBar__list
and Navbar__item
, there is also no need to add such deep nesting with the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the reviews
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How deep nesting should be, two or three?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say one level of "elements" in the "block".
In css you can select items by element, no need for tons of classes, for example .navbar__list a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thx.
</li> | ||
</ul> | ||
</div> | ||
<div className="NavBarNarrow"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...Narrow
sounds like a modificator to me, probably NavBar--narrow
would be better
src/styles/cards-list.scss
Outdated
flex-wrap: wrap; | ||
justify-content: flex-start; | ||
margin: auto; | ||
padding: 0px 40px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0
is sufficient, no need to add px
here
|
||
} | ||
|
||
@media (min-width: 601px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to add 2 media queries here, standard approach is mobile-first so you add styles that will be applied normally and then modify them with query filter withmin-width: 601px
https://trello.com/c/B761DdST