-
Notifications
You must be signed in to change notification settings - Fork 4k
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
first try - search bar #4553
base: master
Are you sure you want to change the base?
first try - search bar #4553
Conversation
GrzegorzLasota
commented
Oct 18, 2024
- DEMO LINK
- TEST REPORT LINK
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.
Great job on your pull request! 🎉 While there are a few minor suggestions regarding CSS practices and alt text descriptions, they don't impact the core functionality of your code. Keep up the good work and consider these suggestions for future improvements!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<img | ||
alt="search-icon" |
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.
The alt attribute should describe the image content. 'search-icon' is not descriptive enough. Consider using a more descriptive alt text, such as 'magnifying glass icon for search'.
<img | ||
alt="search-icon" |
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.
The alt attribute should describe the image content. 'search-icon' is not descriptive enough. Consider using a more descriptive alt text, such as 'magnifying glass icon for search'.
/* add styles here */ | ||
@font-face { | ||
font-family: Avenir; | ||
src: url(/src/fonts/Avenir-Book.ttf) format('opentype') tech(color-COLRv1); |
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.
The tech(color-COLRv1)
part of the src
property is not standard CSS. If you intended to use a specific technology or format, ensure it's supported by browsers or remove it.
|
||
body { | ||
margin: 0; | ||
padding: 20px; |
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.
Consider using only top or bottom padding to avoid potential margin collapse issues. The checklist suggests being consistent with margins and padding.
.lab-first img { | ||
width: 19px; | ||
height: 19px; | ||
margin-left: 26px; |
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.
Consider using only margin-left or margin-right to avoid potential margin collapse issues. The checklist suggests being consistent with margins.
.lab-second img { | ||
width: 11px; | ||
height: 11px; | ||
margin-left: 13px; |
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.
Consider using only margin-left or margin-right to avoid potential margin collapse issues. The checklist suggests being consistent with margins.