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

HTML CSS Popup Practice #65

Merged
merged 2 commits into from
Aug 8, 2022
Merged

HTML CSS Popup Practice #65

merged 2 commits into from
Aug 8, 2022

Conversation

dimonlakhin
Copy link
Contributor

@dimonlakhin dimonlakhin commented Aug 4, 2022

HTML CSS Popup

Demo | Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

@HannaSyn HannaSyn self-requested a review August 5, 2022 09:36
Copy link
Contributor

@HannaSyn HannaSyn left a comment

Choose a reason for hiding this comment

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

Wow! Great design!
It is optional - but if you make this page responsive - it would be awesome practice for you.
Let's improve you code, see my comments below.

<img class="nav-item__icon" src="./img/shapes.png" alt="" />
</label>
<div class="popup">
<ul class="container-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, avoid numbers in class names, use instead BEM

padding: var(--size-5);
}

label {
Copy link
Contributor

@HannaSyn HannaSyn Aug 5, 2022

Choose a reason for hiding this comment

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

Please, avoid to style by HTML tags, read more here
Add to labels class and use it for styling, also for footer

@dimonlakhin dimonlakhin requested a review from HannaSyn August 6, 2022 19:53
@HannaSyn
Copy link
Contributor

HannaSyn commented Aug 8, 2022

Great job!

@HannaSyn HannaSyn merged commit 7c0a8bf into kottans:main Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants