-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
solution #2966
base: master
Are you sure you want to change the base?
solution #2966
Conversation
Patryk-Buczkowski
commented
May 24, 2023
- DEMO LINK
- TEST REPORT LINK
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.
Left one comment on the code structure, will look into css after you deploy the demo.
<link rel="stylesheet" href="./style.css"> | ||
</head> | ||
<body> | ||
<div class="container"> |
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.
Generally you should have an empty line only between siblings of a parent not parent and child,
<div>
<span>first child</span>
<span>first child</span>
</div>
also like here when you have exact same elements next to each other then don't do any empty lines between them, like in this example:
<ul>
<li>text</li>
<li>text</li>
<li>text</li>
<li>text</li>
</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.
Almost done, move the styling higher
margin: 0; | ||
} | ||
|
||
.container { |
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.
Generally you are doing a mobile first approach here, where you first style for screen smaller than 600px, that's good however when doing so logically these style for screen smaller than 600px should be declared before @media (min-width:600px)
, move these lines beginning at line 53 after * { box-sizing: border-box; }
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.
Done :D