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

Module2 task2 #3

Closed
wants to merge 10 commits into from
Closed

Module2 task2 #3

wants to merge 10 commits into from

Conversation

@don-kamaz
Copy link

don-kamaz commented Feb 8, 2020

No description provided.

Copy link
Owner

efiand left a comment

Пока закончу, мало ли сколько времени ты будешь самоотводиться, продолжим после решения этих замечаний.

<div class="pic">
<b class="pic__title">Заинтересовались?</b>
<p class="pic__text">
<div class="map">

This comment has been minimized.

Copy link
@efiand

efiand Feb 8, 2020

Owner

Рекомендация: называть вещи своими именами. Это же не про карту (карта тут - это только часть содержимого).
Это поиск гостиницы, поэтому класс было бы уместнее назвать, например, search-hotel.

source/twig/pages/form.twig Outdated Show resolved Hide resolved
@@ -10,7 +10,7 @@
<p class="main__text">
Помогите нашим отелям стать лучше! оставьте отзыв о них, а также о посещенных вами достопримечательностях
</p>
<h2 class="main__title">Представьтесь:</h2>
<h3 class="main__title">Представьтесь:</h3>
<form action="https://echo.htmlacademy.ru" method="POST">
<div class="data">

This comment has been minimized.

Copy link
@efiand

efiand Feb 8, 2020

Owner

Рекомендация: делать названия классов в БЭМ-блоке формы.

@@ -10,7 +10,7 @@
<p class="main__text">
Помогите нашим отелям стать лучше! оставьте отзыв о них, а также о посещенных вами достопримечательностях
</p>
<h2 class="main__title">Представьтесь:</h2>
<h3 class="main__title">Представьтесь:</h3>
<form action="https://echo.htmlacademy.ru" method="POST">

This comment has been minimized.

Copy link
@efiand

efiand Feb 8, 2020

Owner

Нужен какой-то класс, такие крупные элементы оставлять безымынными это не совсем правильно

Mikhail Belovolov added 4 commits Feb 11, 2020
@efiand efiand self-assigned this Feb 11, 2020
Copy link
Owner

efiand left a comment

Работа слабая даже по меркам четвертого занятия базового интенсива. Подумай о моем предложении начать с нуля, но нормально, без скаканий по верхам, а хорошо погрузившись в основы и закрепив их в своем сознании.
Если так худо зашла разметка с нескольких попыток интенсива, вряд ли стили зашли существенно лучше. А нам не просто стилизовать, но еще с адаптивом. Ты просто потеряешь очередные месяцы.

</div>
<div class="contact-information">
<fieldset>
<legend> </legend>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Секрет успеха в нашем деле - в том числе и стремление сделать все за меньшее число подходов. То есть сразу текст из заголовка переносить в legend, не теряя его по дороге.

<div class="data">
<label>
Имя*:
<input type="text" name="name">

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Советую повторить курсы, там рассказывалось в том числе и про placeholder

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

И про то, как сделать обязательные поля, там тоже рассказывалось. Лейблы же не просто так со звездочками.

<div class="contact-information">
<fieldset>
<legend> </legend>
<div class="data">

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Див нужен только там, где он нужен. Тут никакой сетки нет ни на каком брейкпоинте. Пока ты не начал стилизовать, не пиши несемантических тегов.

</label>
</div>
<div class="contact-information">
<fieldset>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Расставь классы всем тегам по БЭМ (БЭМ-блок - форма)

Опишите подробно все свои восторги
</textarea>
</div>
<p>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

А где вообще лейблы? Почему там лейблы родители, а тут параграфы? В чем прикол делать одинаковое по-разному?

@@ -84,16 +85,18 @@
Красные скалы
</label>
</div>
<f/ieldset>
</fieldset>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Вот такие косяки и помогает валидатор выявить. Но лучше прокачивать внимательность к деталям, для этого нужно постоянное внутреннее чувство ответственности за свой труд.

<input type="number" name="">
</p>
<p>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Странно, почему там добавил параграфы (не обязательно, но и ошибкой не будет) - а тут снес? Делай все как можно единообразнее всегда.

@@ -36,42 +36,36 @@
</fieldset>

<fieldset>
<legend> </legend>
<legend class="title">Контактная информация: </legend>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Да, вот так лучше, но мы же БЭМ используем, зачем в одном и том же проекте и БЭМ, и не БЭМ одновременно? Куда еще пойдут легенды кроме как в форму? Никуда. Следовательно, это не самостоятельные БЭМ-блоки, а БЭМ-элементы.

</div>
</fieldset>

<fieldset>
<legend> </legend>
<legend class="title">Ваше общее впечатление: </legend>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Зачем два пробела между тегом и атрибутом.
И зачем пробел перед закрывающим тегом?
Вся эта грязь в совокупности хоть немного но влияет на восприятие, и вот с таким кодом уже трудно работать, его трудно воспринимать, о нем трудно думать, и реализация какого-то дополнения скорее всего выйдет костыльной.

<p class="radio-button">
<input type="radio" name="#" value="#" checked>
<input type="radio" name="#" checked>

This comment has been minimized.

Copy link
@efiand

efiand Feb 11, 2020

Owner

Зачем ты сначала написал значения, потом убрал? Где это ты нагуглил, что у радиокнопок не должны передаваться значения?
Пройди-ка ты курсы и базовый интенсив прокодь за лекторами, что толку БЭМ писать не зная собственно верстки.

@efiand

This comment has been minimized.

Copy link
Owner

efiand commented on source/twig/pages/form.twig in 38d177c Feb 14, 2020

Сделай уже наконец БЭМ-блок формы!
.form хотя бы, почему у тебя типичные "дети" формы - элементы страницы? Они же без формы не существуют, зачем ниже они все main__?

@efiand

This comment has been minimized.

Copy link
Owner

efiand commented on source/twig/pages/form.twig in 38d177c Feb 14, 2020

Все, что как-то касается эмоций, будет в пределах основного содержимого страницы такое? Сомневаюсь. Я даже более чем уверен, что textarea про что-то отличное от эмоция была бы точно такой же. Да, надо мыслить сущностями, но не контентными, а интерфейсными, компонентными. Важно не то, что это эмоции, а что это блок с сообщением в форме, так и называй классы примерно.

@efiand

This comment has been minimized.

Copy link
Owner

efiand commented on source/twig/pages/form.twig in 38d177c Feb 14, 2020

По этой логике вообще можно кроме main__ названий не делать )) ведь внутри страницы же всё лежит )))

@efiand efiand closed this Feb 25, 2020
@efiand efiand removed their assignment Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.