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.

Sign up for GitHub

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

Jump to bottom

Sadoharu html css popup #8

Merged
merged 9 commits into from
Jul 29, 2022
Merged

Sadoharu html css popup #8

merged 9 commits into from
Jul 29, 2022

Conversation

Sadoharu
Copy link
Contributor

@Sadoharu Sadoharu commented Jul 25, 2022 •
edited
Loading

Hooli-style Popup

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

@HannaSyn
Copy link
Contributor

Great job!
See my comments below

@HannaSyn
Copy link
Contributor

HannaSyn commented Jul 26, 2022 •
edited
Loading

Please, follow all the instructions from task, you missed some of them:

  1. All interactive elements must be marked as such on hover - header menu items included.
  2. Also I can't focus on Hide button with keyboard.

HannaSyn
HannaSyn requested changes Jul 26, 2022
View reviewed changes
submissions/sadoharu/html-css-popup/index.html Outdated Show resolved Hide resolved
submissions/sadoharu/html-css-popup/style.css Outdated Show resolved Hide resolved
submissions/sadoharu/html-css-popup/style.css Show resolved Hide resolved
submissions/sadoharu/html-css-popup/style.css Outdated Show resolved Hide resolved
submissions/sadoharu/html-css-popup/style.css Outdated Show resolved Hide resolved
@Sadoharu
Copy link
Contributor Author

Please, review, again.

@Sadoharu Sadoharu requested a review from HannaSyn July 27, 2022 15:08
@HannaSyn
Copy link
Contributor

Please, review, again.

You did a great work!
Let's fix one more small but important detail - when you tab through navigation, it would be great to have possibility after opening the popup menu, immediately tab to the first icon of the menu, and not go further along the navigation list in header. Also, after clicking on Show more, be able to continue following the list of icons, and only after the popup icons, continue navigation in the header.

A-Ostrovnyy
A-Ostrovnyy requested changes Jul 27, 2022
View reviewed changes
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!

submissions/sadoharu/html-css-popup/index.html Outdated Show resolved Hide resolved
submissions/sadoharu/html-css-popup/style.css Outdated Show resolved Hide resolved
submissions/sadoharu/html-css-popup/style.css Outdated Show resolved Hide resolved
@Sadoharu
Copy link
Contributor Author

okay. I fix this. pls review

@Sadoharu Sadoharu requested a review from A-Ostrovnyy July 27, 2022 19:52
A-Ostrovnyy
A-Ostrovnyy requested changes Jul 28, 2022
View reviewed changes
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!
You still have too specific CSS selectors, try to reduce their specificity.

submissions/sadoharu/html-css-popup/index.html Outdated Show resolved Hide resolved
@Sadoharu
Copy link
Contributor Author

i tried to fix it, pls review

@Sadoharu Sadoharu requested a review from A-Ostrovnyy July 28, 2022 17:08
A-Ostrovnyy
A-Ostrovnyy requested changes Jul 28, 2022
View reviewed changes
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!
But for icons better an empty alt attribute)

@Sadoharu
Copy link
Contributor Author

done

@Sadoharu Sadoharu requested a review from A-Ostrovnyy July 28, 2022 19:19
A-Ostrovnyy
A-Ostrovnyy approved these changes Jul 29, 2022
View reviewed changes
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.

Well done

@A-Ostrovnyy A-Ostrovnyy merged commit a189bc3 into kottans:main Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HannaSyn HannaSyn HannaSyn approved these changes

@A-Ostrovnyy A-Ostrovnyy A-Ostrovnyy approved these changes

Assignees
No one assigned
Labels
task-PopUp
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sadoharu @HannaSyn @A-Ostrovnyy