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

add task solution #4486

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

add task solution #4486

wants to merge 5 commits into from

Conversation

RareHelix
Copy link

mateacademy-ai-mentor
mateacademy-ai-mentor suggested changes Sep 24, 2024
View reviewed changes
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Hey there! It looks like there are a few issues that need your attention before we can move forward. 🛠️ The relative paths for content URLs need to be corrected, a unit for the blur radius should be added to the 'box-shadow' property, and a fallback font family would be a good addition. Also, let's make sure the HTML is nicely formatted with proper indentation and line breaks for readability. Keep up the good work, and with these changes, your code will be even better! 👍

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/index.html Outdated
Comment on lines 19 to 42
<body>
<input
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>
<div class="search-container">
<form
action="/"
method="get"
class="search-bar"
>
<div class="icon">
<input
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
class="search-input big"
/>
</div>
</form>

<input
type="text"
data-qa="hover"
placeholder="Try “Los Angeles“"
/>
<input
type="text"
data-qa="hover"
placeholder="Los Angeles"
class="search-input small"
/>
</div>
Copy link

Choose a reason for hiding this comment

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

Remember about correct indentation between parent and child elements. The elements inside the tag should be indented with 2 spaces.

src/index.html Outdated
Comment on lines 21 to 25
<form
action="/"
method="get"
class="search-bar"
>
Copy link

Choose a reason for hiding this comment

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

Keep your attributes correctly formatted. The attributes of the

tag should start on new lines with proper 2-space indentation.

src/index.html Outdated
Comment on lines 27 to 32
<input
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
class="search-input big"
/>
Copy link

Choose a reason for hiding this comment

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

Keep your attributes correctly formatted. The attributes of the tag should start on new lines with proper 2-space indentation.

Copy link
Author

Choose a reason for hiding this comment

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

it is in the code

src/index.html Outdated
Comment on lines 36 to 41
<input
type="text"
data-qa="hover"
placeholder="Los Angeles"
class="search-input small"
/>
Copy link

Choose a reason for hiding this comment

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

Keep your attributes correctly formatted. The attributes of the tag should start on new lines with proper 2-space indentation.

src/style.css Outdated

body {
margin: 0;
font-family: Avenir, sans-serif;
Copy link

Choose a reason for hiding this comment

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

Consider adding a fallback font family for Avenir in case it fails to load. For example, 'font-family: Avenir, Arial, sans-serif;'. This ensures text is still readable with a default font.

src/style.css Outdated
border-radius: 4px;
font-weight: 300;
transition: border-color 0.3s;
box-shadow: 0 0 7 rgb(205, 205, 205);
Copy link

Choose a reason for hiding this comment

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

The 'box-shadow' property value seems to be missing a unit for the blur radius. It should be something like 'box-shadow: 0 0 7px rgb(205, 205, 205);'.

src/style.css Outdated
margin-top: 20px;
height: 42px;
font-size: 14px;
box-shadow: 0 0 7 rgb(217, 216, 216);
Copy link

Choose a reason for hiding this comment

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

The 'box-shadow' property in the .small class should also include a unit for the blur radius, similar to the .search-input class.

src/style.css
.search-input:focus {
background:
linear-gradient(#ffff, #f6f6f7),
url(../src/images/Search.svg) no-repeat 30px;
Copy link

Choose a reason for hiding this comment

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

The URL for the background image in the .search-input:focus selector should be relative to the CSS file. If the images folder is on the same level as the src folder, the correct path would be './images/Search.svg'.

src/style.css Outdated

.icon::before {
/* background: url(../src/images/Search.svg) no-repeat; */
content: url(../src/images/Search.svg);
Copy link

Choose a reason for hiding this comment

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

The content URL in the .icon::before selector should be relative to the CSS file as well. Make sure the path is correct based on the project's folder structure.

mateacademy-ai-mentor
mateacademy-ai-mentor suggested changes Sep 25, 2024
View reviewed changes
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Hey there! It looks like there are a few areas in the code that could be improved. While the functionality might not be impacted, it's important to address issues like using relative paths for URLs, being consistent with margins, not fixing container sizes, providing fallbacks for fonts, using semantic tags, and maintaining proper formatting for readability. These changes will help make your code more robust and maintainable. Keep up the good work, and with a bit of tweaking, your code will be even better! 🌟

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/index.html Outdated
Comment on lines 20 to 52
<div class="search__container">
<form
action="/"
method="get"
class="search-bar"
data-qa="big"
>
<div class="icon">
<input
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
class="search__input big"
/>
</div>
</form>

<input
type="text"
data-qa="hover"
placeholder="Try “Los Angeles“"
/>
<form
action="/"
method="get"
class="search-bar"
data-qa="small"
>
<div class="icon-small">
<input
type="text"
data-qa="hover"
placeholder="Los Angeles"
class="search__input small"
/>
</div>
</form>
</div>
Copy link

Choose a reason for hiding this comment

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

Add empty lines between multiline sibling blocks of HTML to improve readability.

src/index.html Outdated
Comment on lines 27 to 33
<div class="icon">
<input
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
class="search__input big"
/>
Copy link

Choose a reason for hiding this comment

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

Remember about correct indentation between parent and child elements. The element inside the

should be indented by 2 spaces.

src/index.html Outdated
Comment on lines 43 to 49
<div class="icon-small">
<input
type="text"
data-qa="hover"
placeholder="Los Angeles"
class="search__input small"
/>
Copy link

Choose a reason for hiding this comment

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

Remember about correct indentation between parent and child elements. The element inside the

should be indented by 2 spaces.

src/index.html
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>
<div class="search__container">
Copy link

Choose a reason for hiding this comment

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

Use semantic tags where possible. For example, the search container could be a

or depending on its context within the page.

src/index.html Outdated
class="search-bar"
data-qa="big"
>
<div class="icon">
Copy link

Choose a reason for hiding this comment

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

Class names should represent the meaning of the content, not the styles or tag names. The class 'icon' could be more descriptive.

src/style.css Outdated
Comment on lines 27 to 36
padding-left: 45px;
border: 1px solid #e1e7ed;
border-radius: 4px;
font-weight: 300;
transition: border-color 0.3s;
box-shadow: 0 0 7px rgb(232, 230, 230);
font-family: inherit;
position: relative;
margin-left: 10px;
margin-right: 10px;
Copy link

Choose a reason for hiding this comment

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

Be consistent with your margins. You have both left and right margins set on '.search__input'. Consider sticking to either top and bottom or left and right margins for consistency and to avoid potential margin collapse issues.

src/style.css Outdated
Comment on lines 35 to 36
margin-left: 10px;
margin-right: 10px;
Copy link

Choose a reason for hiding this comment

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

The checklist suggests not to fix container sizes. Here, setting both left and right margins on '.search__input' might lead to unexpected behavior when resizing the window or on different screen sizes. Consider using padding within the parent container or setting a max-width instead.

src/style.css Outdated
Comment on lines 44 to 46
margin-top: 20px;
height: 70px;
font-size: 16px;
Copy link

Choose a reason for hiding this comment

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

The classes '.big' and '.small' are fixing the height of the input fields, which could lead to overflow or accidental scroll bars if the content exceeds the height. Consider using padding and line-height to control the size and let the content dictate the height.

src/style.css Outdated
Comment on lines 50 to 54
margin-top: 20px;
height: 42px;
font-size: 14px;
box-shadow: 0 0 7px rgb(229, 228, 228);
text-shadow: 0 3px 4px #bbbec2;
Copy link

Choose a reason for hiding this comment

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

The '.small' class is adding both a margin-top and a box-shadow. Remember that it's recommended to use margins consistently (either top or bottom, but not both) and to not fix container sizes.

src/style.css Outdated
Comment on lines 74 to 81
content: url(/src/images/Search.svg);
position: absolute;
top: 55px;
z-index: 10;
transform: translateY(-50%);
left: 25px;
height: 19px;
width: 19px;
Copy link

Choose a reason for hiding this comment

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

Using absolute URLs for content in the '::before' pseudo-element might cause issues if the directory structure changes or when moving the project to a different domain. Consider using relative paths instead.

mateacademy-ai-mentor
mateacademy-ai-mentor approved these changes Sep 30, 2024
View reviewed changes
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Great effort! 👍 Remember to address the CSS issues mentioned: ensure container sizes are content-driven, be consistent with margins, include fallback fonts, avoid using the * selector for better performance, and follow the proper indentation and formatting guidelines. Keep up the good work and use these tips to enhance your code in future projects! 💡

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/index.html
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>
<div class="search__container">
Copy link

Choose a reason for hiding this comment

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

Consider adding an empty line before this multiline sibling block for better readability, as per the checklist.

src/index.html
Comment on lines +21 to +25
<form
action="/"
method="get"
class="search-bar"
data-qa="big"
Copy link

Choose a reason for hiding this comment

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

The attributes of the form tag should start on a new line with 2-space indentation related to the tag, and the tag’s closing bracket should be on the same level as the opening one.

src/index.html
Comment on lines +27 to +32
<div class="icon-big">
<input
type="text"
data-qa="keypress"
placeholder="Try &#34;Los Angeles&#34;"
class="search__input big"
Copy link

Choose a reason for hiding this comment

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

The attributes of the input tag should start on a new line with 2-space indentation related to the tag, and the tag’s closing bracket should be on the same level as the opening one.

src/index.html
data-qa="hover"
placeholder="Try “Los Angeles“"
/>
<form
Copy link

Choose a reason for hiding this comment

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

Add an empty line before this multiline sibling block to improve readability, according to the checklist.

src/index.html
Comment on lines +37 to +41
<form
action="/"
method="get"
class="search-bar"
data-qa="small"
Copy link

Choose a reason for hiding this comment

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

The attributes of the form tag should start on a new line with 2-space indentation related to the tag, and the tag’s closing bracket should be on the same level as the opening one.

src/index.html
Comment on lines +43 to +48
<div class="icon-small">
<input
type="text"
data-qa="hover"
placeholder="Try &#34;Los Angeles&#34;"
class="search__input small"
Copy link

Choose a reason for hiding this comment

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

The attributes of the input tag should start on a new line with 2-space indentation related to the tag, and the tag’s closing bracket should be on the same level as the opening one.

src/style.css
Comment on lines +13 to +15
* {
box-sizing: border-box;
}
Copy link

Choose a reason for hiding this comment

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

Don't use the * selector as it impacts performance. Set styles only for elements that require them.

src/style.css
Comment on lines +17 to +19
body {
margin-top: 0;
font-family: Avenir, Arial, sans-serif;
Copy link

Choose a reason for hiding this comment

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

Remember to use fallback fonts - alternative font-family in case the main one doesn't work. Although you have a fallback font here, it's good practice to include a generic font family as well, such as 'serif' or 'sans-serif'.

src/style.css
}

body {
margin-top: 0;
Copy link

Choose a reason for hiding this comment

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

Be consistent with your margins. Add only top or bottom margins, but not both, to avoid potential margin collapse. Since you're setting margin-top, ensure that you're not setting margin-bottom on any elements that would be adjacent vertically.

src/style.css
Comment on lines +22 to +27
.search__container {
display: flex;
flex-direction: column;
align-items: stretch;
width: 100%;
}
Copy link

Choose a reason for hiding this comment

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

Don't fix container size (if there is no such a requirement). Let the content size dictate it. If 'width: 100%' is not a requirement, consider removing it to allow the content to determine the size of the container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateacademy-ai-mentor mateacademy-ai-mentor mateacademy-ai-mentor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RareHelix @mateacademy-ai-mentor