-
-
Notifications
You must be signed in to change notification settings - Fork 184
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 #233
Html css popup #233
Conversation
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. 😒 |
Hi, yes i checked my code according to all the parameters specified in the recommendation |
Fixed alt and added aria-label
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
Sorry for the late response.
…nged the description in some alt tags 4. Added adaptive
Hi, I fixed the comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress!
<li class="header__menu_block"> | ||
<a href="#" class="header__menu_block_link"> | ||
<img src="popup-icons/bell.png" alt="Sign up for updates" class="header__img"> | ||
</a> | ||
</li> | ||
|
||
<li class="header__menu_block"> | ||
<a href="#" class="header__menu_block_link"> | ||
<img src="popup-icons/account.png" alt="Go to your account page" class="header__img"> | ||
</a> | ||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about these alts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tell me what this error is, I can't figure it out on my own (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote them more succinctly. Need to remove alt tags from these icons? But without the voiceover element, they will be invisible to the screen reader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look, for images inside the link in the popup you set an empty value for alt
attribute but for images in the link, you set the text value for the alt
attribute. Try to use a single approach for all images.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ні, I fixed it
I hope my solution to this comment is correct =)
…inctly described alt tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
* HTML-CSS-Popup is done * changed the list navigation * Fixed alt and added aria-label Fixed alt and added aria-label * 1. Removed extra empty lines 2. Added an alt tag to all images 3. Changed the description in some alt tags 4. Added adaptive * 1. fixed behavior of elements when hovering and focusing 2. more succinctly described alt tags * alt tag correction
HTML-CSS-Popup
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.