-
Notifications
You must be signed in to change notification settings - Fork 1
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
Solve 01-basics #2
Conversation
Solve 01-basics/20-weather
Добавляю преподавателя (@ShGKme) для код-ревью. |
Solve 02-basics-2/20-broken-map Solve 02-basics-2/30-calculator Solve 02-basics-2/40-marked-emails Solve 02-basics-2/50-selected-meetup
Solve 02-basics-2
Проверьте, пожалуйста, ваше решение, не все тесты прошли (PR не будет принят до тех пор, пока все добавленные задачи не будут решены). |
Решение было обновлено, посмотрим что скажет @ShGKme |
Проверьте, пожалуйста, ваше решение, не все тесты прошли (PR не будет принят до тех пор, пока все добавленные задачи не будут решены). |
01-basics/10-create-app/script.js
Outdated
}; | ||
createApp({ | ||
name: 'todayDate', | ||
template: 'Сегодня ' + new Date().toLocaleDateString(navigator.language, options), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Так определять шаблон некорректно. Здесь не инструментами Vue шаблоне описано, что должно выводиться, а сгенерирован сам шаблон на JS. Вместо этого шаблон должен быть фиксированной строкой, и уже внутри шаблона можно использовать директивы и текстовую интерполяцию для определения содержимого.
- Так собрать шаблон в основном формате Vue компонентов - SFC - вообще не возможно
- Значение шаблона определяется в момент описания компонента, а не в момент его работы. Так, если страница будет открыта сегодня, но компонент впервые срендерится только завтра - он всё равно выведет вчерашнюю дату
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если это было сделано, потому что navigator.language
не доступен в шаблоне - лучше использовать переменную/функцию, описанную в setup
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Загрузил исправления
Solve 03-components/20-UiClock Solve 03-components/30-removable-emails Solve 03-components/40-UiCounter Solve 03-components/50-weather-components Fix 01-basics/10-create-app Fix 01-basics/20-weather
Добавляю преподавателя (@ShGKme) для код-ревью. |
Исправлены указанные ошибки. |
Solve 03-components/20-UiClock Solve 03-components/30-removable-emails Solve 03-components/40-UiCounter Solve 03-components/50-weather-components Fix 01-basics/10-create-app Fix 01-basics/20-weather
Решение было обновлено, посмотрим что скажет @ShGKme |
Solve 03-components/20-UiClock Solve 03-components/30-removable-emails Solve 03-components/40-UiCounter Solve 03-components/50-weather-components Fix 01-basics/10-create-app Fix 01-basics/20-weather
Решение было обновлено, посмотрим что скажет @ShGKme |
Проверьте, пожалуйста, ваше решение, не все тесты прошли (PR не будет принят до тех пор, пока все добавленные задачи не будут решены). |
Solve 04-sfc/20-MeetupCover-style-v-bind Solve 04-sfc/30-UiStretch
Решение было обновлено, посмотрим что скажет @ShGKme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Все задачи приняты
export default defineComponent({ | ||
name: 'WeatherCard', | ||
|
||
props: { | ||
card : { | ||
type: Object, | ||
required: true | ||
}, | ||
icons: { | ||
type: Object, | ||
required: true | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пропустил при проверке.
Сейчас для использования многих компонентов (включая компонент списка карточек) нужно передавать массив иконок. Это неудобно - пользователю даже списка, нужно иметь этот список, его кастомизации пока не требуется. Такая возможность может быть плюсом, если она дополнительная и опциональная (есть иконки по умолчанию). Но это неудобный интерфейс компонента, если для вывода списка нужно передавать объект иконок для разных погодных условий.
</div> | ||
</li> | ||
</ul> | ||
<WeatherCardList :cardsList="wData" :icons="icons"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По стайл гайду принято пропсы (и события) в шаблоне использовать в kebab-case
Solve 01-basics/10-create-app
Solve 01-basics/20-weather
Solve 02-basics-2/10-counter
Solve 02-basics-2/20-broken-map
Solve 02-basics-2/30-calculator
Solve 02-basics-2/40-marked-emails
Solve 02-basics-2/50-selected-meetup