Add Hooli-popup-task homework #85
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Create dir submissions/LuckyDnepr/Hooli-popup-task Copy index.html & styles-menu.css.
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
AnnaStoyano
reviewed
Aug 8, 2022
LuckyDnepr
added a commit
to LuckyDnepr-LearningStage/Kottans-Hooli-style-popup
that referenced
this pull request
Aug 9, 2022
LuckyDnepr
added a commit
to LuckyDnepr-LearningStage/Kottans-Hooli-style-popup
that referenced
this pull request
Aug 10, 2022
Many changes in index.html and styles-menu.css. This checkbox should be part of <nav>. kottans/frontend-2022-homeworks#85 (comment) Resolved You can use <label> tag instead of hide hide checkbox under div kottans/frontend-2022-homeworks#85 (comment) Resolved Only 1 nav tag should be on the page. kottans/frontend-2022-homeworks#85 (comment) Resolved Add hover state kottans/frontend-2022-homeworks#85 (comment) Resolved not need to use additional div. You can just style a tag as you need. A large number of nesting complicates the readability of the code. kottans/frontend-2022-homeworks#85 (comment) Resolved no need to explicitly specify the value of tabindex. a tag support keyboard interaction because a tag is focusable. kottans/frontend-2022-homeworks#85 (comment) Add alt attribute. Read more alt kottans/frontend-2022-homeworks#85 (comment) Resolved The same. Please check all popup elements kottans/frontend-2022-homeworks#85 (comment) Resolved Please remove empty lines kottans/frontend-2022-homeworks#85 (comment) Resolved Don't use html selector in this case. If you want to apply the styles for all element you can use * (all), body or parent element like .nav kottans/frontend-2022-homeworks#85 (comment) Resolved Please see the video kottans/frontend-2022-homeworks#85 (comment) Resolved No need to add many empty lines kottans/frontend-2022-homeworks#85 (comment) Resolved Reorder styles property. Something like display width height margin ... kottans/frontend-2022-homeworks#85 (comment) Resolved remove empty lines kottans/frontend-2022-homeworks#85 (comment) Resolved Do you need this style ? I don't see any usage of nav_item_marker class kottans/frontend-2022-homeworks#85 (comment) Resolved try not to use html tag selectors ( p, span, div, ... ) for styling. You can apply this style for all .nav_menu_item_link_container or add class for p. kottans/frontend-2022-homeworks#85 (comment) Resolved Let's make some changes. Please, use a template similar to the one in the example: It doesn't have to be the same, but have similar navigation elements like links and popup to have opportunity navigate using keyboard. kottans/frontend-2022-homeworks#85 (comment) Resolved Please, also add prettier to your ide. It will format spaces in code kottans/frontend-2022-homeworks#85 (comment) Resolved
Many changes in index.html and styles-menu.css.
|
Great job =) Lets make some changes |
AnnaStoyano
reviewed
Aug 10, 2022
AnnaStoyano
reviewed
Aug 10, 2022
AnnaStoyano
reviewed
Aug 10, 2022
AnnaStoyano
reviewed
Aug 10, 2022
LuckyDnepr
added a commit
to LuckyDnepr-LearningStage/Kottans-Hooli-style-popup
that referenced
this pull request
Aug 11, 2022
style flex-direction: row-reverse; reverse your row and when user starts moving using the keyboard it`s start from the end (right->left). This is not a common practice. Or is it a expected feature? kottans/frontend-2022-homeworks#85 (comment) Resolved When the user opens the popup and presses "TAB", the focus is not on the first link (Application). It's is not expected. Expected navigation using keyboard: User open popup and click TAB - focus on the first link in popup User not open popup and click TAB - focus on the bell You can fix it by changing tag order in html. (Example: move bell and user a tags under the popup). kottans/frontend-2022-homeworks#85 (comment) Resolved remove empty line kottans/frontend-2022-homeworks#85 (comment) Resolved The same code kottans/frontend-2022-homeworks#85 (comment) Resolved
style flex-direction: row-reverse; reverse your row and when user starts moving using the keyboard it`s start from the end (right->left). This is not a common practice. Or is it a expected feature? #85 (comment) Resolved When the user opens the popup and presses "TAB", the focus is not on the first link (Application). It's is not expected. Expected navigation using keyboard: User open popup and click TAB - focus on the first link in popup User not open popup and click TAB - focus on the bell You can fix it by changing tag order in html. (Example: move bell and user a tags under the popup). #85 (comment) Resolved remove empty line #85 (comment) Resolved The same code #85 (comment) Resolved
AnnaStoyano
reviewed
Aug 11, 2022
AnnaStoyano
reviewed
Aug 11, 2022
|
Hello @HannaSyn @A-Ostrovnyy ! Could you please also take a look? |
A-Ostrovnyy
requested changes
Aug 11, 2022
| <img | ||
| class="nav_icons" | ||
| src="./icons/popup-button.png" | ||
| alt="popup-menu" |
There was a problem hiding this comment.
Read this article to find out how and when populating alt attribute
There was a problem hiding this comment.
I read the article. Focusing on using screen readers, I think I fixed the code in the part about setting the attribute.
Although without experience, some fixes may be unnecessary. Please comment on it.
There was a problem hiding this comment.
First of all, don't resolve comments., it's for reviewers)
Icon is a decorative image which means alt attribute must be empty.)
There was a problem hiding this comment.
I need to unresolved all comments?
About on icons, I will resolve this...
There was a problem hiding this comment.
About unresolved comments, right now all good, I mean that for future homeworks.
There was a problem hiding this comment.
W3C standard saying:
When an [a] element that creates a [hyperlink], or a [button] element, has no textual content but contains one or more images, the [alt] attributes must contain text that together convey the purpose of the link or button.
Other opinion in W3C:
Do not specify irrelevant alternate text when including images intended to format a page, for instance, alt="red ball" would be inappropriate for an image that adds a red ball for decorating a heading or paragraph. In such cases, the alternate text should be the empty string (""). Authors are in any case advised to avoid using images to format pages; style sheets should be used instead.
I tested my page in NVDA, and I think, my new changes are correct.
Please review.
There was a problem hiding this comment.
Impressive 😮
In browser focus going to interactive elements, and we can implement some strategies:
- Add
aria-labelattribute to the interactive element and the screen reader will read him; - Add text along with the image but hide him using
visually-hiddenutility class;
No matter what strategy you choose be sure it solves your current task.
LuckyDnepr
added a commit
to LuckyDnepr-LearningStage/Kottans-Hooli-style-popup
that referenced
this pull request
Aug 11, 2022
Lets make some small changes: You apply class logo for div tag. div is block element by default. kottans/frontend-2022-homeworks#85 (comment) Resolved Logo should be a link and leads user to the home page kottans/frontend-2022-homeworks#85 (comment) Resolved Avoid adding empty tags to commit kottans/frontend-2022-homeworks#85 (comment) Resolved When you have nested navigation better to use HTML lists for the first and other levels. kottans/frontend-2022-homeworks#85 (comment) Resolved Read article to find out how and when populating alt attribute kottans/frontend-2022-homeworks#85 (comment) Resolved Why you add id here? kottans/frontend-2022-homeworks#85 (comment) Resolved
You apply class logo for div tag. div is block element by default. #85 (comment) Resolved Logo should be a link and leads user to the home page #85 (comment) Resolved Avoid adding empty tags to commit #85 (comment) Resolved When you have nested navigation better to use HTML lists for the first and other levels. #85 (comment) Resolved Read article to find out how and when populating alt attribute #85 (comment) Resolved Why you add id here? #85 (comment) Resolved
Let hide "Show more" button after click it. Make alt attributes to "img" tags. Reading some information about screenreaders, test Popup-menu using NVDA screenreader. And finally using W3C HTML 4.01 Specification paragraph 13.8.
LuckyDnepr
requested review from
A-Ostrovnyy and
AnnaStoyano
and removed request for
AnnaStoyano
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hooli-popup-task
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.