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 #38

Merged
merged 5 commits into from
Aug 1, 2022
Merged

Conversation

natalia-artsiukh
Copy link
Contributor

Hooli-style Popup

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

@kasionio
Copy link
Contributor

Great job! Nice formatting, which is pretty easy to read, that's cool. The menu is also looking good

Seems there is still some room for UI and accessibility improvements.

  1. I can't reach the popup menu using just a keyboard.
  2. Hover effect "cursor: pointer" should appear only on clickable elements(buttons, links, etc.), not on the whole container.
  3. "More" button should take 100% width of the popup container by design, seems more containers will be needed in code for this :)
  4. Please take a look at the screenshot. Seems that the container needs to be better centered and also should have just one padding, the second looks redundant.
  5. To prevent jumping on elements if you want to add some padding on hover can be done like so:
    .element { padding: 1px solid transparent; } .element: hover { padding: 1px solid SOME_COLOR; }

image

Don't hesitate to ask if something is not clear ;)

    I removed the container around the menu icons and the need for centering and extra stroke disappeared. Thanks to this, hover and focus are applied to the icons themselves. Added transparent borders to elements so they don't jump when focusing on them. The "more" button was removed from the grid and the display block was used - it became the entire width of the popup. I put the second part of the pop-up in a separate container. Added a border between the initial content and additional content. Slightly reworked the indents.
@natalia-artsiukh
Copy link
Contributor Author

I made the necessary changes to my files. please check)
I couldn't make a re-request review because I don't have reviewers

Copy link
Collaborator

@A-Ostrovnyy A-Ostrovnyy 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!
See my comments below.

@kasionio kasionio self-requested a review July 31, 2022 19:04
Copy link
Contributor

@kasionio kasionio left a comment

Choose a reason for hiding this comment

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

Great job! Last steps left

    I have added an unordered list. Removed all alt values. Made an shorthand background. Rounded the values of the alpha channel in rgba. Changed the name of the color to rgb module.
    Replaced pixel values with relative ones. Fixed number of display properties in popup-items. Made the page responsive.
Copy link
Collaborator

@A-Ostrovnyy A-Ostrovnyy left a comment

Choose a reason for hiding this comment

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

Looks good to me.
@kasionio what's your input about it?

@kasionio
Copy link
Contributor

kasionio commented Aug 1, 2022

@A-Ostrovnyy, agree. @natalia-artsiukh, congrats 🎉

@A-Ostrovnyy A-Ostrovnyy merged commit 04ec3cc into kottans:main Aug 1, 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