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

00-intro - настройка репозитория #1

Merged
merged 25 commits into from
Oct 1, 2024

Conversation

Ziesel777
Copy link
Contributor

No description provided.

@jsru-1
Copy link
Contributor

jsru-1 commented Sep 22, 2024

Спасибо за отправленный пулриквест, теперь @ShGKme должен запустить тесты.

@Ziesel777 Ziesel777 closed this Sep 22, 2024
@Ziesel777 Ziesel777 reopened this Sep 22, 2024
@jsru-1
Copy link
Contributor

jsru-1 commented Sep 24, 2024

Добавляю преподавателя (@ShGKme) для код-ревью.

@jsru-1 jsru-1 requested a review from ShGKme September 24, 2024 17:59
Comment on lines +3 to +4
const FormatDate = defineComponent({
name: 'format-date',
Copy link

Choose a reason for hiding this comment

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

Имя обычно дают также в PascalCase

Copy link
Contributor Author

@Ziesel777 Ziesel777 Oct 2, 2024

Choose a reason for hiding this comment

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

Приучили до этого на протяжении 4х лет к кебаб кейсу в имени компонента, чтоб совпадало с именем селектра рутового элемента компонента, т.к. разделение шаблона на компоненты в основном происходило по БЭМ. Объяснялось тем, что в итоге такой компонент легче найти, как визуально в испекторе, так и в IDE по поиску (типо выдернул из инспектора копипастом, вставил в IDE, всё нашлось)

Так что тут наверно скорее предпочтения отдельно взятой организации, не сказать что это обычно в паскаль кейсе или в кебаб кейсе.

Copy link

Choose a reason for hiding this comment

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

В шаблоне компонент действительно часто используют в kebab-case. Но не в JS части, не в имени.

https://vuejs.org/style-guide/rules-strongly-recommended.html#component-name-casing-in-js-jsx

Comment on lines +24 to +36
props: {
meetup: {
type: Object,
default: {
// - `title` — заголовок митапа
// - `image` — ссылка на изображение обложки митапа
// - `description` — описание митапа
// - `agenda` — массив с программой митапа
// - `organizer` — организатор митапа
// - `place` — место проведения митапа
// - `date` — дата проведения митапа
},
},
Copy link

Choose a reason for hiding this comment

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

Если компонент не имеет смысла или не функционирует без пропса - он должен быть явно описан, как обязательный через required: true. В противном случае либо компонент просто не может нормально работать, либо выводит пустые блоки.

Злоупотребление ?. оператором не решает эту проблему, а скорее усугубляет, так как прячет настоящую ошибку за выводом пустых элементов.

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

Copy link

Choose a reason for hiding this comment

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

Также у мутабельного типа пропса не должен быть непосредственно объект (массив) в качестве значения по умолчанию. Так у всех экземпляров этого класса по умолчанию в пропсе будет лежать ссылка на один и тот же объект. Вместо этого должна быть функция создания такого объекта () => ({})

Copy link

Choose a reason for hiding this comment

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

Не смотря на предыдущий комментарий, значение {} здесь в принципе не корректно в качестве значения по умолчанию. {} нельзя считать корректным объектом с данными митапа, это не частный случай данных.

Copy link

Choose a reason for hiding this comment

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

Аналогично в 03-components/50-weather-components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Приму, но обратите внимание на комментарии на будущее.

@jsru-1 jsru-1 merged commit a0da583 into js-tasks-ru:master Oct 1, 2024
1 check passed
@Ziesel777
Copy link
Contributor Author

Ок, буду иметь ввиду.

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.

3 participants