Skip to content

Dev js#1

Open
evgdudareff wants to merge 30 commits into
masterfrom
dev-js
Open

Dev js#1
evgdudareff wants to merge 30 commits into
masterfrom
dev-js

Conversation

@evgdudareff

Copy link
Copy Markdown
Owner

ПР на проверку

Comment thread .babelrc
@@ -0,0 +1,10 @@
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Заметки по недоработкам визуальным:

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

  2. У тебя не убрано базовое поведение селекта (выбора города) — за счет чего на айфоне выбрать город практически нельзя.

  3. Было бы хорошо для формы иметь поддержку клавиатуры (например в том же селекте выбор сделать возможным с помощью клавиатуры)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

При ресайзе выпадашка "едет"
image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

Сейчас не очень понятна какая ошибка — лучше сообщать это пользователю хоть как-то

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

По пунктам:

  1. Исправлено. Также упростил код, доработал структуру кода и файлов js.
  2. Теперь доступно и по Tab (с ios тоже доступно переключение, но каретку там не победил). Также упростил код, доработал структуру кода и файлов js.
  3. Смотри пункт 2.
  4. При ресайзе выпадашка "едет" - исправил. Для Tablet/Desktop использую только css position. При пересечении границы для Mobile один раз перерисовываются.
    При resize в границе Mobile - динамически меняется положение.
  5. При скролле также элемент не остается на выбранном месте, а бегает за тобой, что не очень вписывается в макет -исправил.
  6. Сейчас не очень понятна какая ошибка — лучше сообщать это пользователю хоть как-то - добавил. Теперь если пользователь не верно ввел данные в поле и ушел с него, то показывается попап с ошибками.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

  1. попап лучше держать с описанием ошибки до момента, пока пользователь не поставит фокус на поле или уйдет с него. Иначе получается как на скриншоте.

  2. Если вызвать селект и убрать с поля фокус, лучше, чтобы выпадашка пропадала. Иначе она загораживает пространство и мешает работать с сайтом

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

А зачем при самовывозе адрес? :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Пока ситуация с resize еще не решилась
image

@evgdudareff evgdudareff Jan 14, 2020

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

  1. Теперь предупреждение показывается только тогда, когда пользователь работает с полем.
  2. Сделал, чтобы пропадал select при клике не на него.
  3. А зачем при самовывозе адрес? :) - теперь при самовывозе секция адреса скрывается.
  4. с resize решили, что по это по макету

Comment thread css/blocks/form/form.less
justify-content: center;
align-items: center;
display: none;
.orderForm {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

где-то стили в-таком-стиле-описаны, где-то вСовершенноДругом. Лучше держать единый стиль, так как это структурирует код прививает его единое восприятие

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

У меня часть блоков "осталась" от Леонида Феськова. Посыл понял.

Comment thread css/common/input/input.less Outdated

.input {
&_invalid {
border: 1px solid #c52929;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

не достаточно ли только border-color менять?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Исправил

Comment thread css/common/select/select.less Outdated
}

.select-field__option {
display: none;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

не работает в мобилках

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

именно на option теге

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Переделал select, уже не актуально.Но сделал выбор доставки доступным через клавиатуру(заменил display:none на clip: rect (0 0 0 0) ).

Comment thread html/orderForm.html
<form class="form" action="/" method="post">
<div class="columns-wrapper">
<div class="form__button-close">
<button class="button-close-icon button-close">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

А зачем на кнопку два разных блока навешивать? это вполне можно соорудить через кнопку + блок-иконка в ней

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Согласен, исправил.

Comment thread js/orderForm/hideOrderForm.js Outdated
export const hideOrderForm = e => {
let target = e.target;

while (!target.classList.contains("orderForm")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

closest можно использовать https://developer.mozilla.org/ru/docs/Web/API/Element/closest


if (currentForm) {
currentForm.classList.remove("orderForm_active");
setTimeout(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

лучше подписка на событие

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

сделал через событие


//Контакная информация и расширенный адрес - обязательные поля.
//проверить, все ли требуемые элементы прошли валидацию
for (let i = 0; i < inputs.length; i++) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if (inputs.some())

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

some возвращает true/false, мне же нужно не только понять, какое поле не прошло валидацию, но и получить само поле, чтобы сделать input.validation.checkValidities(input).
В целом идея понятна, но не слишком подходит сейчас...

Comment thread js/orderForm/prepareToSubmit.js Outdated
//СМС уведомления
let smsNotification = false;
if (document.querySelector("input[name=smsNotification]:checked")) {
smsNotification = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

сейчас тебе нужно частично дособирать данные руками. можно ли это свести к минимуму?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

свёл к минимуму. Собираю необходимые поля и сразу же сохраняю теперь в объект с данными формы

Comment thread js/orderForm/showOrderForm.js Outdated
});

//Обработать селектор города
const select = document.querySelector(".select__select-field");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

в этом файле собралась вся логика, которую не получилось отнести к чему-то конкретному. Это и логика самой формы (скрыть форму) и выборы города и так далее.
Лучше разделить на разные файлы, а в orderForm.js вызывать нужные методы

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

разбил на файлы и функции

}
}

.select_active > .input-select {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

В БЭМ не рекомендуется модифицировать один блок через другой

}
}

function getClassName(elem) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Кажется, что этой функции не место в fieldValidation
И главная проблема ее в текущем виде — тебе придется постоянно ее дорабатывать — больно :(

break;
}
//если клик на кнопку "Закрыть", то скрыть форму
if (target.classList.contains("form__button-close")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

мы обсуждали на занятии, почему на css классы не стоит завязываться, и лучше использовать js классы

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

здесь и везде

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