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

Разбивка компонента «Конструктор пиццы» на мелкие компоненты #2

Merged
merged 23 commits into from
Jul 16, 2021

Conversation

Shirokuiu
Copy link

@Shirokuiu Shirokuiu commented Jul 4, 2021

@keksobot keksobot changed the title Module2 task1 Разбивка компонента «Конструктор пиццы» на мелкие компоненты Jul 5, 2021
Copy link

@vas11yev1work vas11yev1work left a comment

Choose a reason for hiding this comment

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

В целом все хорошо, жду правки по комментариям.
Были места в которых недочеты были идентичны с теми, что я уже описывал ранее в других файлах, поэтому я не стал дублировать.

Comment on lines 8 to 25
@onDoughChange="updateCurrentDough($event)"
/>

<BuilderSizeSelector
:sizes="sizes"
@onSizeChange="updateCurrentSize($event)"
/>

<BuilderIngredientsSelector
:sauces="sauces"
:ingredients="ingredients"
@onSauceChange="updateCurrentSauce($event)"
@onCountUpdate="updateIngredientsPrice($event)"
/>

<BuilderPriceCounter
:total-price="totalPrice"
@onIngredientDrop="updateIngredientsPrice($event)"

Choose a reason for hiding this comment

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

В таких вызовах, когда у нас в аргумент функции передается $event, его можно не писать
То есть, например @onDoughChange="updateCurrentDough($event)" будет равносильно @onDoughChange="updateCurrentDough"

Comment on lines 18 to 20
classMods: {
type: Array,
},

Choose a reason for hiding this comment

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

Критерий Б3. Нужно задать либо required: true либо default
Для массивов default можно задать вот так

props: {
  arr: {
    type: Array,
    default: () => []
  }
}

Ссылка на ишью: vuejs/vue#1032 (comment)

Choose a reason for hiding this comment

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

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

Ссылка на документацию: https://ru.vuejs.org/v2/guide/class-and-style.html#%D0%98%D1%81%D0%BF%D0%BE%D0%BB%D1%8C%D0%B7%D0%BE%D0%B2%D0%B0%D0%BD%D0%B8%D0%B5-%D1%81-%D0%BA%D0%BE%D0%BC%D0%BF%D0%BE%D0%BD%D0%B5%D0%BD%D1%82%D0%B0%D0%BC%D0%B8

Comment on lines 33 to 36
classMods: {
type: Array,
required: false,
},

Choose a reason for hiding this comment

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

Критерий Б3. Нужно задать либо required: true либо default
Для массивов default можно задать вот так

props: {
  arr: {
    type: Array,
    default: () => []
  }
}

Ссылка на ишью: vuejs/vue#1032 (comment)

Comment on lines +20 to +22
classMods: {
type: Array,
},

Choose a reason for hiding this comment

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

Критерий Б3. Нужно задать либо required: true либо default
Для массивов default можно задать вот так

props: {
  arr: {
    type: Array,
    default: () => []
  }
}

Ссылка на ишью: vuejs/vue#1032 (comment)

Choose a reason for hiding this comment

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

Вот в таком случае уже более уместно использование пропса для передачи классов

Comment on lines 11 to 14
:value="value"
:is-checked="isChecked"
:name="name"
@onRadioChange="onSauceChange($event)"

Choose a reason for hiding this comment

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

У нас есть директива v-model, которую мы можем привязать к какому-то инпуту. RadioButton не исключение, то есть обычному radio мы можем задать директиву v-model. Но эту директиву мы так же можем задать и компоненту, настроив внутри компонента нужные свойства: принимающее значение и emit.

Ссылка на документацию: https://ru.vuejs.org/v2/guide/components-custom-events.html#%D0%9D%D0%B0%D1%81%D1%82%D1%80%D0%BE%D0%B9%D0%BA%D0%B0-v-model-%D1%83-%D0%BA%D0%BE%D0%BC%D0%BF%D0%BE%D0%BD%D0%B5%D0%BD%D1%82%D0%B0

({ multiplier: multiplierMap }) => multiplier === multiplierMap
).value,
},
isChecked: index === 1,

Choose a reason for hiding this comment

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

Аналогично в normalizeDoughs

Comment on lines +3 to +9
<input
type="radio"
:name="radioName"
:value="value"
:checked="isChecked"
@change="onRadioChange(value)"
/>

Choose a reason for hiding this comment

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

У нас есть директива v-model, которую мы можем привязать к какому-то инпуту. RadioButton не исключение, то есть обычному radio мы можем задать директиву v-model. Но эту директиву мы так же можем задать и компоненту, настроив внутри компонента нужные свойства: принимающее значение и emit.

Ссылка на документацию: https://ru.vuejs.org/v2/guide/components-custom-events.html#%D0%9D%D0%B0%D1%81%D1%82%D1%80%D0%BE%D0%B9%D0%BA%D0%B0-v-model-%D1%83-%D0%BA%D0%BE%D0%BC%D0%BF%D0%BE%D0%BD%D0%B5%D0%BD%D1%82%D0%B0

Comment on lines 20 to 22
const transferData = JSON.parse(
dataTransfer.getData(DATA_TRANSFER_PAYLOAD)
);

Choose a reason for hiding this comment

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

Сюда можно передать уже созданную переменную payload

}}</span>

<AppCounter
:class-mods="['counter--orange', 'ingridients__counter']"

Choose a reason for hiding this comment

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

Ошибка ingredients

Copy link
Author

Choose a reason for hiding this comment

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

в стилях такой же класс
Снимок экрана 2021-07-09 в 09 21 52

Comment on lines 48 to 55
value: undefined,
});
},

dec() {
this.$emit("onCountUpdate", {
action: countAction.DEC,
value: undefined,

Choose a reason for hiding this comment

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

По-моему нет никакого смысла использовать value: undefined. Но если все же нужно передавать пустое значение, то лучше использовать null


computed: {
totalPrice() {
return (
Copy link

Choose a reason for hiding this comment

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

в расчете стоимости должен учитываться размер

</label>

<div class="content__constructor">
<div class="pizza pizza--foundation--big-tomato">
Copy link

Choose a reason for hiding this comment

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

в этом блоке должно отражаться состояние пиццы (в зависимости от настроек теста, соуса, ингридиентов)

Copy link
Author

Choose a reason for hiding this comment

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

Согласно пунктам ТЗ этого раздела, реализовывать данный функционал не нужно
Снимок экрана 2021-07-15 в 23 18 36

Copy link

Choose a reason for hiding this comment

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

Частично - только про пункт 3.7, не так ли?
Про пункт 3.2 этого не сказано, а именно он описывает эту логику

<template>
<div class="counter">
<button
type="button"
Copy link

Choose a reason for hiding this comment

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

кнопки должны отключаться когда нельзя уменьшить/увеличить количество ингредиентов

Copy link
Author

Choose a reason for hiding this comment

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

Опять же, в рамках этого модуля этот функционал реализовывать не нужно
Снимок экрана 2021-07-15 в 23 22 28

Copy link

Choose a reason for hiding this comment

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

3.2.3 Изменение ингредиентов
В случае если количество ингредиента на пицце равно 3-м, кнопка + становится неактивной, также отключается возможность переноса ингредиента на пиццу.

В случае если количество ингредиента на пицце равно 0, кнопка - становится неактивной.

Copy link

@Djaler Djaler left a comment

Choose a reason for hiding this comment

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

Отсутствует часть функционала по ТЗ.
Часть правок, оставленных на предыдущем ревью, не исправлена

@Shirokuiu
Copy link
Author

Какая именно часть ТЗ указанного в этом модуле не реализована ?
И какие именно правки по прошлым замечаниям не исправлены ?
P.S. Если про v-model, то мне пока не удобно ее реализовывать. Критерии академии меня в этом не ограничивают. Это было утверждено с предыдущим наставником

@Shirokuiu
Copy link
Author

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

@Djaler
Copy link

Djaler commented Jul 15, 2021

Если про v-model, то мне пока не удобно ее реализовывать. Критерии академии меня в этом не ограничивают. Это было утверждено с предыдущим наставником

Ну, я же об этом не в курсе, в обсуждениях на гитхабе этого нет)

@Shirokuiu
Copy link
Author

Shirokuiu commented Jul 15, 2021

Ну, я же об этом не в курсе, в обсуждениях на гитхабе этого нет)

Согласен) Просто наставник сказал что его не будет и поэтому редиректнул мрчик

@Djaler
Copy link

Djaler commented Jul 16, 2021

Можно мерджить, думаю

@keksobot keksobot merged commit 1115e57 into htmlacademy-vue:master Jul 16, 2021
@Shirokuiu
Copy link
Author

Done

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