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

Merged
merged 6 commits into from
Aug 5, 2022
Merged

html-css-popup #48

merged 6 commits into from
Aug 5, 2022

Conversation

KonstantinOkhorzin
Copy link
Contributor

HTML і CSS практика: Hooli-style Popup

Demo |
Code base

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! See some comments below =)

</li>
<li class="popap__item">
<a href="#" class="popap__link">
<img class="popap__link-icon" src="icons/mail.png" alt="icon">
Copy link
Contributor

Choose a reason for hiding this comment

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

For icons it's better set an empty alt attribute, read more here

<li class="navbar__item">
<a href="#" class="navbar__link">Images</a>
</li>
<li class="navbar__item popap">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, replace popap with popup everywhere in project, it is a typo.

<img class="popap__trigger-icon" src="icons/popup-button.png" alt="icon">
</label>
<div class="popap__body">
<input class="popap__input-more" id="popap__more" type="checkbox">
Copy link
Contributor

Choose a reason for hiding this comment

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

You need move this input because when you navigate with tab, focus should be at icons first, and on More after icons.

Added attribute tabindex because when you navigate with tab, focus should be at icons first, and on More after icons.
@KonstantinOkhorzin
Copy link
Contributor Author

I fixed all. Please, check the changes.

@HannaSyn
Copy link
Contributor

HannaSyn commented Aug 4, 2022

Great work! Almost done
When I navigate with tab, I can see "hidden" icons without click on More button, but expected, that only click on the button should show them.

Took away the opportunity to interact with "hidden" icons when I navigate with a tab.
@KonstantinOkhorzin
Copy link
Contributor Author

I fixed this bug. Please, check the changes.

@HannaSyn HannaSyn merged commit ea688f1 into kottans:main Aug 5, 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