Hooli-style Popup #123

Yanyshpolska · 2022-08-11T17:01:41Z

Hooli-style Popup

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

github-actions · 2022-08-16T10:59:02Z

Hey!

Congratulations with your PR! 😎😎😎

Please, be sure you haven't followed common mistakes.

Also, be aware, that if you would silently ignore this recommendation, a mentor may think that you are still working on fixes. And your PR will not be reviewed. 😒

Yanyshpolska · 2022-08-16T15:23:01Z

Thank you for providing chapter "Typical mistakes" for "popup task". It was helpful and I was able to get rid of all the unnecessary UI/LI tags. Also I have changed button MORE - now it becomes a button LESS when clicked.
Please, review.

kasionio · 2022-08-16T18:06:03Z

Please try to reach Less button from the keyboard, now it looks impossible :)

Yanyshpolska · 2022-08-16T20:15:24Z

Hi! Last hour I have tried to solve this problem. The button is actually "more" button, so you can reach it on position between "always visible items" and "more items" items. And it indeed seems weird than you must press "tab" back to focus on the "Less" button. The "~" and "+" (as I understand, and as I tried) can reach only the next element in the document. If I add new checkbox button to the end of item list - it will nor reach upper element.
So, I have tried add radio button with "More" and "Less" items. But it behaved in the same manner as with the checkbox, so this solution didn't work here for me.
Also I have tried to set order of flex element for checkbox to sent it back, but it didn't change the real order of objects, so in it also didn't work.
I know this can be done with JS but I didn't find solution how to do it purely in HTML/CSS only.
Please point me in the right direction :)

Yanyshpolska · 2022-08-24T05:00:29Z

Hi! As I understand, "alt" descriptions for icons in the popup menu are not necessary, so I've deleted them. I'm still waiting for advice about the "Less" button. Or I can remove it and hide the "More" button when pressing it.

kasionio

Nice job, good code! Let's improve it a bit. Take a look at the comments and screenshot. Also, need to add a pointer on hover effect on the popup button and More button. Focus on popup button is not centered

kasionio · 2022-08-25T19:36:35Z

submissions/yanyshpolska/Hooli-style Popup/css/style.css

@@ -0,0 +1,213 @@
+/* base styles */


please remove comments, they are not needed, code should be readable by its nature :D

Agreed. My bad. I'll delete them.

kasionio · 2022-08-25T19:36:45Z

submissions/yanyshpolska/Hooli-style Popup/css/style.css

@@ -0,0 +1,213 @@
+/* base styles */
+* {


kasionio · 2022-08-25T19:42:44Z

submissions/yanyshpolska/Hooli-style Popup/css/style.css

+}
+/* ===============Menu text items======================= */
+.menu__item_text {
+  /* display: block; */


all commented out code should be deleted

kasionio · 2022-08-25T19:46:37Z

submissions/yanyshpolska/Hooli-style Popup/css/style.css

+}
+.menu__item_checkbox_label {
+  position: absolute;
+  height: 1.715em;


Try to use also rem :)
https://essential-addons.com/elementor/css-design-guide-difference/
https://www.quora.com/What-is-the-difference-between-em-rem-and-px-in-CSS

Please read the articles (these or any others) and add at least a couple of rems, to the corresponding classes. Rem is more used nowadays than em

kasionio · 2022-08-25T19:47:09Z

submissions/yanyshpolska/Hooli-style Popup/index.html

+          <img
+            class="menu__item_picture_img"
+            src="./img/bell.png"
+            alt="Alerts"


https://www.a11yproject.com/posts/alt-text/

For this particular case, I believe that usage of the "alt" is reasonable: in case the picture is not loaded the user is still able to see picture caption which is aligned with advice from this article:
"Use the alt attribute for images that are used as more than decoration."

Yes, it is reasonable, but the value is not correct

kasionio · 2022-08-25T19:47:47Z

submissions/yanyshpolska/Hooli-style Popup/index.html

+      <nav class="menu__items">
+        <a class="menu__item_text menu__item" href="#">Mail</a>
+        <a class="menu__item_text menu__item" href="#">Images</a>
+


it is not needed to leave spaces in HTML file

kasionio · 2022-08-25T19:49:12Z

submissions/yanyshpolska/Hooli-style Popup/index.html

+  </head>
+  <body>
+    <header>
+      <nav class="menu__items">


for plural naming better use list word for example, because item and items looks pretty the same and you need to pay much attention to see the difference

kasionio · 2022-08-25T19:51:49Z

submissions/yanyshpolska/Hooli-style Popup/index.html

+          <div class="popup__wrapper">
+            <div class="popup__items">
+              <a href="#" class="popup__item">
+                <div class="popup__item_image_container">


you can try to remove this div or text span and reduce the amount of tags, or wrap text and img in one div and use flex.

Here I use div-container-for-picture in purpose because all pictures have different sizes and this approach allows me to deal with it: regardless of the size of the picture, the menu item cells will be the same.

You can specify image width explicitly in corresponding class for images. You can leave it, but this is far from optimal

Yanyshpolska · 2022-08-25T21:44:18Z

Please check the code adjusted according to your comments. I have corrected the focus on the popup button and deleted extra spaces and all comments. Also, I have changed class names.

Yanyshpolska · 2022-08-26T15:30:14Z

Please check the code adjusted according to your comments. I have removed "div-container-for-picture" and changed the "alt" value from "Alerts" to "Notifications".

kasionio

Please add the hover pointer effect on popup button and More button

kasionio · 2022-08-29T19:23:34Z

submissions/yanyshpolska/Hooli-style Popup/css/style.css

+  font-size: 100%;
+  line-height: 1;
+  font-size: 14px;
+  -ms-text-size-adjust: 100%;


what does this property mean?

kasionio · 2022-08-29T19:24:05Z

submissions/yanyshpolska/Hooli-style Popup/css/style.css

+a {
+  text-decoration: none;
+}
+ul li {


you can leave just ul or li here

kasionio · 2022-08-29T19:27:04Z

submissions/yanyshpolska/Hooli-style Popup/index.html

+          />
+        </a>
+        <a class="menu__item_picture menu__item" href="#">
+          <img class="menu__item_picture_img" src="./img/logo.jpg" alt="User" />


https://www.w3schools.com/tags/att_img_alt.asp

Changed the "alt" value from "User" to "Profile".

The text should explain where the link goes if the image is inside an <a> element
So, it should be "Go to Profile page"

Please make corresponding changes to alts

Yanyshpolska · 2022-08-29T20:03:37Z

Added hover pointer and removed unnecessary rows.

…2022-homeworks into popupMenu

kasionio

Last steps ;)

kasionio · 2022-08-30T18:07:21Z

submissions/yanyshpolska/Hooli-style Popup/css/style.css

+}
+.menu__item_checkbox_label {
+  position: absolute;
+  height: 1.715em;


Please read the articles (these or any others) and add at least a couple of rems, to the corresponding classes. Rem is more used nowadays than em

Yanyshpolska · 2022-08-30T19:57:04Z

Hi! Thanks for your comments. Reworked as advised. Please review.

kasionio

nope, please read the articles and add rems where you think it is NEEDED, not everywhere.

Yanyshpolska · 2022-08-31T18:53:55Z

Hi Illia. Of course, I read the articles which you suggested to me. I also read other articles and watched some videos about units of measure. I clearly understand the differences in EM and REM size units. But I don't know the "best practice" of using them. I believe that the experience of use will come with time and with the number of implemented projects.
This popup menu project is my first project. I have used EM units here only to have some practice with them. The first version of popup menu was written with the PX. In my CSS file I set the font size only one time only for the HTML tag. So I expect the behavior of EM and REM will be the same.
If you have some suggestions for "best practice" solutions with EM REM - I will gladly listen to them. But for now, I can only guess where to use EM or REM units better in this particular project and I did the best I can considering my current knowledge.
According to the articles I read (https://zellwk.com/blog/rem-vs-em/, https://zellwk.com/blog/media-query-units/) I have found some suggestions for using EM and REM: "Size in em if the property scales according to its font-size", "Size everything else in rem." "Use em for media queries." According to your article, I have changed "border-radius" to EM.

kasionio · 2022-08-31T19:24:57Z

Nice answer, fair enough ;) I guess it is time to go to the next tasks 🚀

Please review

73e26e3

A-Ostrovnyy added the task-PopUp label Aug 11, 2022

OleksiyRudenko added task-PopUp and removed task-PopUp labels Aug 16, 2022

Delete lists from nav tag. Change More/Less button

9c84c0a

kasionio self-requested a review August 16, 2022 18:06

A-Ostrovnyy assigned kasionio Aug 18, 2022

deleted alt descriptions for popup menu items

bdb6aa8

hid more button

78c22a3

kasionio requested changes Aug 25, 2022

View reviewed changes

Yanyshpolska added 2 commits August 26, 2022 00:30

adjusted according to review comments

1b11b0f

removed extra spaces

8027cab

Yanyshpolska requested a review from kasionio August 25, 2022 22:23

removed the unnecessary image containers

3abf32f

kasionio requested changes Aug 29, 2022

View reviewed changes

Yanyshpolska added 2 commits August 29, 2022 22:53

added hover pointer and removed unessesary rows

4d1015e

added hover pointer and removed unnecessary rows

6f89fc5

Yanyshpolska requested a review from kasionio August 29, 2022 20:04

Yanyshpolska added 2 commits August 29, 2022 23:54

added display:none to checkbox More button

7961687

Merge branch 'popupMenu' of https://github.com/Yanyshpolska/frontend-…

7435267

…2022-homeworks into popupMenu

kasionio requested changes Aug 30, 2022

View reviewed changes

changed em to rem. corrected alt text

f17084e

Yanyshpolska requested a review from kasionio August 30, 2022 19:57

kasionio requested changes Aug 30, 2022

View reviewed changes

changed border-radius units to em

a581f99

kasionio approved these changes Aug 31, 2022

View reviewed changes

kasionio merged commit 322e39f into kottans:main Aug 31, 2022

Hooli-style Popup #123

Hooli-style Popup #123

Conversation

Yanyshpolska commented Aug 11, 2022

Hooli-style Popup

github-actions bot commented Aug 16, 2022

Yanyshpolska commented Aug 16, 2022

kasionio commented Aug 16, 2022

Yanyshpolska commented Aug 16, 2022

Yanyshpolska commented Aug 24, 2022

kasionio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yanyshpolska commented Aug 25, 2022

Yanyshpolska commented Aug 26, 2022

kasionio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasionio Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Yanyshpolska commented Aug 29, 2022

kasionio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yanyshpolska commented Aug 30, 2022

kasionio left a comment

Choose a reason for hiding this comment

Yanyshpolska commented Aug 31, 2022

kasionio commented Aug 31, 2022

kasionio left a comment •

edited

Loading

kasionio Aug 30, 2022 •

edited

Loading