-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat(DateInput): add prop onApply #7929
base: master
Are you sure you want to change the base?
feat(DateInput): add prop onApply #7929
Conversation
size-limit report 📦
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
e2e tests |
👀 Docs deployed
Commit 0ac2935 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7929 +/- ##
=======================================
Coverage 95.30% 95.30%
=======================================
Files 378 378
Lines 11196 11212 +16
Branches 3735 3739 +4
=======================================
+ Hits 10670 10686 +16
Misses 526 526
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
Отличный ресерч. Выглядит как будто так и должно было быть с самого начала. 🥇
Хм, на сколько это теперь фича, или может быть это уже breaking change раз у нас onChange
теперь не всегда срабатывает при изменении значения (при enableTime
). Нормально ли это будет в v6 закидывать, или лучше только v7 добавить?
В целом, мы уже решили основные проблемы, которые мешали в v6, и эту фичу можно только в v7 добавить, как думаешь?
Да, я согласен, что лучше добавить уже в v7(можно в 7.1.0 например) |
# Conflicts: # packages/vkui/src/components/Calendar/Calendar.tsx
Описание
В текущей реализации
DateInput
с включенным флагомenableTime
событиеonChange
срабатывает при каждом выборе даты в календаре. Это создает неудобства в использовании компонента. Необходимо оптимизировать работуonChange
и добавить отдельный колбэк для подтверждения выбора даты.Изменения
После анализа решений в популярных UI-библиотеках (например, AntDesign) были внесены следующие изменения в логику работы компонента:
enableTime=true
:onChange
не вызывается при каждом выборе датыDone
, после чего срабатываютonApply
иonChange
Технические изменения:
value
для временного хранения выбранной датыvalue
onApply
для обработки подтверждения выбораДобавлены тесты для проверки новой функциональности
Примечание: В будущем стоит рассмотреть добавление кнопки подтверждения выбора для случая
enableTime=false
иcloseOnChange=false
, так как сейчас в этом режиме нет возможности явно подтвердить выбор даты.Release notes
Улучшения
onApply
, которое срабатывается при нажатии на кнопкуDone
, которая отображается при использовании флагаenableTime
.enableTime
при выборе в календаре не срабатываетсяonChange
. Теперь он сработает, только при нажатии на кнопку"Готово"