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.

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

Develop #4648

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

Develop #4648

wants to merge 13 commits into from

Conversation

Gennadiy99
Copy link

  • DEMO LINK

  • TEST REPORT LINK

  • Icon implemented using background-image CSS property

  • Inputs are written inside of 'form' tag with correctly passed attributes

  • All Typical Mistakes from BEM lesson theory are checked.

  • Code follows all the Code Style Rules ❗️

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.

Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. The data-qa attributes in the HTML are incorrect and need to be adjusted to meet the task requirements. Additionally, the CSS does not ensure the search bar stretches the full width of the container, and the spacing between search bars is not explicitly set. These issues impact the functionality and core requirements of the task. Please make these changes, and feel free to reach out if you need any help! Keep up the great work! 🚀

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

@@ -4,7 +4,7 @@
<meta charset="UTF-8" />
<meta
name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"
content="width=device-width, user-scalable=no, initial-scale=1.0,maximum-scale=1.0,minimum-scale=1.0"

Choose a reason for hiding this comment

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

Consider adding spaces after commas in the content attribute for better readability: content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0".

Copy link
Author

Choose a reason for hiding this comment

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

В задании написано:

  • добавьте атрибут data-qa="big" для большой формы поиска и data-qa="small" для маленькой
  • добавьте атрибут data-qa="keypress" для ввода в большую форму поиска
    Это значит что нужно input добавить атрибут с названием data-qa и получается с 2мя значениеями "big" и "keypress" данная форма записи корректна = data-qa="big keypress"
  1. Кроме того, CSS не гарантирует, что строка поиска растянется на всю ширину контейнер:
    Насколько мне известно свойство .input-bg { width: 100%; } - Должно растягивать элемент на всю ширину родительского блока.
  2. а расстояние между строками поиска не задано явно:
    Расстояние между input заданно через .search-block__item {
    margin-top: 20px;
    }
    Прошу обратить внимание на эти данные.

src/index.html Outdated
Comment on lines 31 to 37
placeholder="Try “Los Angeles“"
/>
<input
class="input-sm input-sm--hover input-sm--focus search-block__item"
type="text"
data-qa="small"
placeholder="Try “Los Angeles“"

Choose a reason for hiding this comment

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

The placeholder attributes for both input fields are identical. If this is not intentional, consider providing different placeholders to distinguish the input fields.

Copy link
Author

Choose a reason for hiding this comment

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

Данные внесены согласно макету.

body {
margin: 0;
font-family: Avenir-Book, sans-serif;
font-weight: 300;

Choose a reason for hiding this comment

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

The font-weight for the body is set to 300, but the defined font faces (Avenir-Book and Avenir-Heavy) only specify normal and bold. Consider adjusting the font-weight to match the available weights.

Copy link
Author

Choose a reason for hiding this comment

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

данных свойств достаточно для body дополнительные свойства шрифта дополняются уже к элементам body ,- "input"

src/style.css Outdated
Comment on lines 52 to 85
box-shadow: 0 4px 3px 5px rgba(0, 0, 0, 0.1);
}

.input-bg--focus:focus {
outline: none;
font-weight: bold;
box-shadow: 0 4px 3px 5px rgba(0, 0, 0, 0.1);
text-shadow: -2px 4px 2px rgba(0, 0, 0, 0.3);
}

.input-sm {
box-sizing: border-box;
max-width: 100%;
width: 100%;
height: 42px;
padding: 12px 12px 10px 33px;

border: none;
border-radius: 4px;
box-shadow: 0 0 6px 3px rgba(0, 0, 0, 0.1);

font-family: inherit;
font-weight: 400;
font-size: 14px;
color: #3d4e61;

background-image: url(images/Search.svg);
background-repeat: no-repeat;
background-size: 11px;
background-position: 13px 50%;
}

.input-sm--hover:hover {
box-shadow: 0 4px 3px 5px rgba(0, 0, 0, 0.1);

Choose a reason for hiding this comment

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

The box-shadow values for the hover and focus states of .input-bg and .input-sm are identical. If you want to provide a distinct visual feedback for these states, consider varying the box-shadow values.

Copy link
Author

Choose a reason for hiding this comment

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

Согласно макету при обоих этих свойствах тень блока остается неизманной, меняется вес шрифта и добавляется тень к шрифту.

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 job on addressing most of the issues in your code! 🎉 Your HTML and CSS files are well-structured and mostly align with the task requirements. However, there are a few areas that need attention:

  1. HTML File:

    • The data-qa attributes for the forms need to be reviewed. Ensure they are correctly set according to the task requirements.
  2. CSS File:

    • The spacing between the search bars should be explicitly set to 20px. Currently, the margin for .search-block is set to 20px 8px, which does not guarantee the required spacing.

These adjustments are important for the functionality and alignment with the task specifications. Please review the comments and make the necessary changes to improve your code. Keep up the good work and continue learning from these experiences! 🚀

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

action="/submit"
method="post"
class="search-block"
data-qa="big"

Choose a reason for hiding this comment

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

The data-qa attribute for the first form is set to big, which does not meet the task requirements. Please ensure that the data-qa attributes are correctly set as per the task description.

action="/submit"
method="post"
class="search-block"
data-qa="small"

Choose a reason for hiding this comment

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

The data-qa attribute for the second form is set to small, which does not meet the task requirements. Please ensure that the data-qa attributes are correctly set as per the task description.

}

.search-block {
margin: 20px 8px;

Choose a reason for hiding this comment

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

The margin for .search-block is set, but it does not explicitly set spacing between search bars. Ensure that the spacing between search bars is explicitly defined as per the task requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants