add task solution #4496
add task solution #4496
Conversation
There was a problem hiding this comment.
Great job on the Pull Request! There are a few minor issues like class naming and responsive design considerations, but none of these critically impact the functionality or security of your code. Remember to use meaningful class names and keep an eye on responsive design principles for future projects. Keep up the good work! 👍💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <html lang="en"> | ||
| <html | ||
| lang="en" | ||
| class="paige" |
There was a problem hiding this comment.
The class attribute value 'paige' does not represent the meaning of the content, it seems to represent a style or tag name. Consider using a class name that reflects the content or purpose of the element.
src/index.html
Outdated
| <input | ||
| type="text" | ||
| id="big-search" | ||
| placeholder="Try 'Los Angeles'" | ||
| class="search-bar__input big" | ||
| data-qa="keypress" | ||
| /> |
There was a problem hiding this comment.
The class name 'big' does not represent the meaning of the content, it seems to represent a style. Use class names that reflect the content or function rather than the appearance.
src/index.html
Outdated
| <input | ||
| type="text" | ||
| id="small-search" | ||
| placeholder="Try 'Los Angeles'" | ||
| class="search-bar__input small" | ||
| /> |
There was a problem hiding this comment.
Similarly to the previous input, the class name 'small' is not descriptive of the content or function. Consider using a more meaningful class name.
| } | ||
|
|
||
| .paige__body { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
Consider adding a fallback font-family. While 'Avenir' is specified, it's a good practice to provide a generic family in case the primary font is not available, for example, 'font-family: Avenir, sans-serif;'.
src/style.css
Outdated
| font-style: normal; | ||
| } | ||
|
|
||
| .paige { |
There was a problem hiding this comment.
The class name '.paige' seems to be a typo or not meaningful. Ensure class names represent the meaning of the content or the role of the element within your design.
src/style.css
Outdated
| background-size: 19px 19px; | ||
| } | ||
|
|
||
| .search-bar__input.big { |
There was a problem hiding this comment.
Setting a fixed 'min-width' can cause content overflow on smaller screens or devices with lower resolution. Consider using responsive units like percentages or viewport units, or employ media queries to handle different screen sizes.
src/style.css
Outdated
| background-position: left 26px center; | ||
| } | ||
|
|
||
| .search-bar__input.small { |
There was a problem hiding this comment.
Similar to the previous comment, setting a fixed 'min-width' here can cause layout issues on smaller screens. It's better to use responsive design techniques to ensure the input field scales appropriately.
src/style.css
Outdated
| .search-bar__input:focus { | ||
| background-color: #fff; | ||
| border-radius: 3px; | ||
| font-family: Avenir-Heavy, sans-serif; |
There was a problem hiding this comment.
The font-family 'Avenir-Heavy' is not defined in any @font-face rule and may not be loaded correctly. You might want to use 'Avenir' and adjust the 'font-weight' property instead.
| } | ||
|
|
||
| .paige { | ||
| margin: 0; |
There was a problem hiding this comment.
Be consistent with your margins. It's recommended to add only top or bottom margins but not both to avoid potential margin collapse issues.
src/index.html
Outdated
| <html lang="en"> | ||
| <html | ||
| lang="en" | ||
| class="paige" |
There was a problem hiding this comment.
| class="paige" | |
| class="page" |
typo
src/index.html
Outdated
| type="text" | ||
| id="small-search" | ||
| placeholder="Try 'Los Angeles'" | ||
| class="search-bar__input small" |
There was a problem hiding this comment.
| class="search-bar__input small" | |
| class="search-bar__input search-bar__input--small" |
small is not a separate class - make it a BEM modificator
src/style.css
Outdated
| } | ||
|
|
||
| .search-bar__input.big { | ||
| min-width: 900px; |
There was a problem hiding this comment.
It seems it's redundant, I guess you can safely remove it
| width: 100%; | ||
| } | ||
|
|
||
| .search-bar__input { |
There was a problem hiding this comment.
Note that placeholder styles might be overwritten with browser default styles:
Use ::placeholder pseudo-class to define styles for placeholder (you can use inherit value for some properties)
https://devtoolstips.org/tips/en/inspect-user-agent-dom/
| } | ||
|
|
||
| .search-bar__input.small:focus { | ||
| background-image: url(images/Search.svg); |
There was a problem hiding this comment.
you should add gradient of focus according to design
Might be helpful:
src/style.css
Outdated
| .search-bar__input:focus { | ||
| background-color: #fff; | ||
| border-radius: 3px; | ||
| font-family: Avenir-Heavy, sans-serif; |
There was a problem hiding this comment.
| font-family: Avenir-Heavy, sans-serif; | |
| font-family: Avenir, sans-serif; |
YaroslavaSemyda
requested review from
TarasHoliuk and
mateacademy-ai-mentor
| font-weight: 700; | ||
| font-style: normal; | ||
| } | ||
|
|
There was a problem hiding this comment.
| body { | |
| margin: 0 8px; | |
| } |
| .search-bar { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
| .search-bar { | |
| width: 100%; | |
| } | |
| .search-bar { | |
| width: 100%; | |
| margin-top: 20px; | |
| } |
src/style.css
Outdated
| .search-bar__input { | ||
| width: 100%; | ||
| outline: none; | ||
| text-align: left; | ||
| background-image: url(images/Search.svg); | ||
| background-repeat: no-repeat; | ||
| color: #3d4e61; | ||
| border: 1px solid #e1e7ed; | ||
| box-shadow: 0 1px 8px rgba(61, 78, 97, 0.1); | ||
| border-radius: 4px; | ||
| background-size: 19px 19px; | ||
| } |
There was a problem hiding this comment.
| .search-bar__input { | |
| width: 100%; | |
| outline: none; | |
| text-align: left; | |
| background-image: url(images/Search.svg); | |
| background-repeat: no-repeat; | |
| color: #3d4e61; | |
| border: 1px solid #e1e7ed; | |
| box-shadow: 0 1px 8px rgba(61, 78, 97, 0.1); | |
| border-radius: 4px; | |
| background-size: 19px 19px; | |
| } | |
| .search-bar__input { | |
| box-sizing: border-box; | |
| width: 100%; | |
| outline: none; | |
| text-align: left; | |
| background-image: url(images/Search.svg); | |
| background-repeat: no-repeat; | |
| color: #3d4e61; | |
| border: 1px solid #e1e7ed; | |
| box-shadow: 0 1px 8px rgba(61, 78, 97, 0.1); | |
| border-radius: 4px; | |
| background-size: 19px 19px; | |
| } |
src/style.css
Outdated
| } | ||
|
|
||
| .search-bar__input.search-bar__input--big { | ||
| margin-top: 20px; |
src/style.css
Outdated
| font-weight: 400; | ||
| line-height: 20px; | ||
| background-size: 11px 11px; | ||
| margin-top: 20px; |
DEMO LINK
TEST REPORT LINK
Icon implemented using background-image CSS property
Inputs are written inside of 'form' tag with correctly passed attributes
[ X] All
Typical MistakesfromBEMlesson theory are checked.[X ] Code follows all the Code Style Rules ❗️