Html & CSS Practice. Popup #43
Conversation
There was a problem hiding this comment.
Good job. But still need some improvements.
-
You missed part of the task, the tricky one :) You need to implement this triangle
-
When a user hovers over the link it becomes "jumping". Try to solve this by adding transparent padding by default
-
The popup menu should be centered on the toggle button (look at the picture below).
-
Please add inner padding to the popup menu, it will look better.
|
|
||
| :root { | ||
| --main-color: lightgrey; | ||
| --bg-color: #fff; |
There was a problem hiding this comment.
Why are you using HEX and RGB models at the same time?
| } | ||
|
|
||
| *, | ||
| *::before, |
There was a problem hiding this comment.
What is the reason to use pseudoelements like so?
|
|
||
| body { | ||
| font-family: "Work Sans", sans-serif; | ||
| background-color: var(--main-color); |
| } | ||
|
|
||
| .header__wrapper { | ||
| max-width: 1800px; |
There was a problem hiding this comment.
It is not looks good on big screens
| } | ||
|
|
||
| .popup__list { | ||
| display: grid; |
There was a problem hiding this comment.
IMHO, it can be done with flex. Seems there is no need to use a grid model here
| <input | ||
| class="add-popup__control" | ||
| type="checkbox" | ||
| id="popup-2" |
There was a problem hiding this comment.
Try not to use names like popup-1 and popup-2. Better to specify the place or which action it handles. For example popup-more or popup-inner. Here it is okay, but imagine if you will need to add more popups on the page.
|
Please try to make equal width for all containers, it will look better.
|
There was a problem hiding this comment.
Nicely done! Please take a look at the screenshots below and look at the comments. This should be the final changes
- The container should be centered.
- Links in the main navigation bar should not have borders on hover. The same for
Morebutton - Try to use
border-radiusproperty to make the design a little bit smoother
| } | ||
|
|
||
| .add-popup { | ||
| position: relative; |
There was a problem hiding this comment.
You have the same property in line 82. No need to duplicate. Also check border: var(--border-hover);
| padding: 0.6rem; | ||
| } | ||
|
|
||
| .add-popup__list-item { |
There was a problem hiding this comment.
I can't find where you use this class in HTML. Please, don't hurry, take your time and review your code. Sometimes you need just to take a one-day rest and make a self-code-review to find some gaps. I know it from my own experience ;)
| border: 1px solid transparent; | ||
| } | ||
|
|
||
| .popup__link-title { |
There was a problem hiding this comment.
Too complicated approach for just a text inside span. Try to optimize your code and play with properties, it can be fun :) I am sure that you don't need flex here, just in the container popup__list-item.
| </div> | ||
| <a class="main__nav-item" href="#"> | ||
| <img src="./images/bell.png" alt="bell" | ||
| /></a> |
There was a problem hiding this comment.
/> missed the right place ;) Move it to corresponding line.
<img src="./images/bell.png" alt="bell" /> or each attribute in a new line
| <img | ||
| class="popup__link-icon" | ||
| src="./images/documents.png" | ||
| alt="doc" |
There was a problem hiding this comment.
Please read about alt attribute and make corresponding changes.
The alt attribute provides alternative information for an image if a user for some reason cannot view it (because of slow connection, an error in the src attribute, or if the user uses a screen reader).
https://www.w3.org/QA/Tips/altAttribute
|
Are you already done with the changes? I see the new commit, but you don't re-request review |
|
Yes, sorry. Just realized that you didn’t receive notifications without me pressing the re-request review button |
html-css-popup
Demo |
Code base