Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

HTML-CSS-practice-popup_task #152

Merged
merged 2 commits into from
Aug 28, 2022
Merged

Conversation

Halv27
Copy link
Contributor

@Halv27 Halv27 commented Aug 14, 2022

HTML-CSS-practice-popup_task

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

@github-actions
Copy link

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. 😒

@Halv27
Copy link
Contributor Author

Halv27 commented Aug 18, 2022

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)) I checked the code, there are no typical mistakes in it.

@yaripey yaripey self-assigned this Aug 21, 2022
Copy link

@yaripey yaripey left a comment

Choose a reason for hiding this comment

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

Hey, nice work! Let's improve it!

First of all, your popup button doesn't have any visual response for focusing it with the keyboard.

Secondly, you cannot get to the More button with the keyboard, focus just jumps from the Documents button to the Bell icon in the top bar.

About SVGs - your way of storing them inside the file is cool, but there could be some pros and cons to that. What's bad is that it messes up with file readability a little and those icons cannot be cached to improve performance. On the other hand, if you store them separately they might be lost somewhere since it creates more HTML requests, so you have to consider different perspectives when working with SVG when you get to real work. The common usage is to store all SVGs in one sprite and to use use. (pun intended)

You can leave SVGs unchanged here, it's fine for the first work.

@Halv27
Copy link
Contributor Author

Halv27 commented Aug 23, 2022

Hey, nice work! Let's improve it!

First of all, your popup button doesn't have any visual response for focusing it with the keyboard.

Secondly, you cannot get to the More button with the keyboard, focus just jumps from the Documents button to the Bell icon in the top bar.

About SVGs - your way of storing them inside the file is cool, but there could be some pros and cons to that. What's bad is that it messes up with file readability a little and those icons cannot be cached to improve performance. On the other hand, if you store them separately they might be lost somewhere since it creates more HTML requests, so you have to consider different perspectives when working with SVG when you get to real work. The common usage is to store all SVGs in one sprite and to use use. (pun intended)

You can leave SVGs unchanged here, it's fine for the first work.

Нi, thank you very much for the review.
I made some changes to make the focus work.
I didn't know about SVSs. Thank you for your response and the article, it was very useful and interesting information. But I didn't change anything this time.

@Halv27 Halv27 requested a review from yaripey August 23, 2022 17:11
@yaripey
Copy link

yaripey commented Aug 28, 2022

Good job!

@yaripey yaripey merged commit b9563cc into kottans:main Aug 28, 2022
@Halv27
Copy link
Contributor Author

Halv27 commented Aug 29, 2022

Good job!

Thanks))

SphericalCat pushed a commit to SphericalCat/frontend-2022-homeworks that referenced this pull request Aug 30, 2022
* git commit -m 'Add html-css-popup_task'

* Add changes to the popup files
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants