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

Компоненты: погода в Средиземье #10

Merged
merged 9 commits into from
Jan 21, 2025

Conversation

luquii2
Copy link

@luquii2 luquii2 commented Jan 2, 2025

No description provided.

@jsru-1
Copy link
Contributor

jsru-1 commented Jan 2, 2025

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

@jsru-1 jsru-1 requested a review from ShGKme January 2, 2025 19:23
@jsru-1
Copy link
Contributor

jsru-1 commented Jan 2, 2025

Решение было обновлено, посмотрим что скажет @ShGKme

@luquii2
Copy link
Author

luquii2 commented Jan 2, 2025

Григорий, подскажите пожалуйста, почему у меня ws ругается на стили и названия в объекте meetup?
Верстка тоже поехала, потому что он скорее всего не видит переменные в стилях
image

@jsru-1
Copy link
Contributor

jsru-1 commented Jan 3, 2025

Решение было обновлено, посмотрим что скажет @ShGKme

@jsru-1
Copy link
Contributor

jsru-1 commented Jan 3, 2025

Решение было обновлено, посмотрим что скажет @ShGKme

@luquii2
Copy link
Author

luquii2 commented Jan 3, 2025

Картинки из задания UiStretch не работают
image

Copy link
Contributor

@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.

Принято, но обратите внимание на комментарий.

Comment on lines +9 to +12
icons: {
type: Object,
required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Этот параметр не должен быть обязательнымicon не должен быть обязательным параметром.

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

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

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

@jsru-1 jsru-1 merged commit 3fb1d9b into js-tasks-ru:master Jan 21, 2025
1 check passed
Comment on lines +39 to +40
<template v-slot:name>{{ data.alert.sender_name }}</template>
<template v-slot:description>{{ data.alert.description }}</template>
Copy link
Contributor

Choose a reason for hiding this comment

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

Обычно используют короткий вариант директив: #name

Comment on lines +10 to +12
<slot name="name" ></slot>:
<br>
<slot name="description" ></slot>
Copy link
Contributor

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.

4 participants