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

Hooli style popup #89

Merged
merged 6 commits into from
Aug 18, 2022
Merged

Hooli style popup #89

merged 6 commits into from
Aug 18, 2022

Conversation

AsaMitaka
Copy link
Contributor

Hooli-style Popup

Code
Demo

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

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.

Good work!
It is optional, but if you want to improve your markup skills - check responsive - it isn't looks good under 400px.
And see my comments below

<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Document</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add title of your page here

<link rel="stylesheet" href="style.css">
</head>
<body>
<nav class="nav">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, wrap navigation by tag header

</li>
<li class="nav__item">
<input type="checkbox" id="menu" class="check__input" tabindex="-1">
<label for="menu">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add possibility to open and close menu with keyboard


<hr>

<input type="checkbox" id="more" class="more__checkbox" tabindex="-1"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add possibility to open hidden icons menu with keyboard

@@ -0,0 +1,183 @@
/* nav block */
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't leave unnecessary comments in files

align-items: center;
}

.nav__ul li a {
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 styling by tags link

<a href="#">Images</a>
</li>
<li class="nav__item">
<!-- <input type="checkbox" id="menu" class="check__input" tabindex="-1"> -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, remove unnecessary comments

outline: 2px solid blue;
}

input[type='checkbox']:focus + label {
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 styling by tags at all, use class names link

.popup {
right: -1em;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line, please

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.

When reviewer types comment on 1 item, it means that you should fix all similar items.

outline: 2px solid blue;
}

.check__input:focus + label {
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 styling by tags at ALL, use class names link

outline: 2px solid blue;
}

.more__checkbox:focus + label {
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 styling by tags at ALL, use class names link

display: block;
}

.popup ul {
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 styling by tags at ALL, use class names link

box-shadow: 0px 0px 22px -1px rgba(0,0,0,0.12);
}

.popup__item a {
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 styling by tags at ALL, use class names link

flex: 0 0 33%;
}

.more__popup__item a {
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 styling by tags at ALL, use class names link

display: block;
}

.more__popup__open a {
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 styling by tags at ALL, use class names link

color: black;
}

.more__popup__open a:hover {
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 styling by tags at ALL, use class names link

@HannaSyn HannaSyn merged commit 9ce3a15 into kottans:main Aug 18, 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.

4 participants