-
Notifications
You must be signed in to change notification settings - Fork 132
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(): Add new <DateRangePicker>
component
#3586
base: next
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
a8b895b
to
23df406
Compare
23df406
to
52fab02
Compare
c3a26e8
to
81b4b52
Compare
8a6fbaf
to
aa9ffd5
Compare
2a24d34
to
df140b9
Compare
393cca9
to
bd1cc39
Compare
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.
Пока посмотрел только часть, без тестов и мобильного вида.
По поведению заметил такую особенность. Если maxDate
меньше выбранной даты в инпуте, то при фокусе на нем Календарь покажет месяц, отличный от того, что выбран. И может быть сложно доскролить до выбранной даты, если она далеко, т.к. выпадашки месяца и года отключены.
Оригинальный DatePicker в этом плане отличается, показывая выбранный месяц.
Так и задумано? Не успел еще со всей аналитикой ознакомиться.
@sashachabin: всё так, это ошибка — поправил!
Start: (props: DateRangePickerFieldWithTypeProps) => <DateRangePickerField {...props} type="start" />, | ||
End: (props: DateRangePickerFieldWithTypeProps) => <DateRangePickerField {...props} type="end" />, |
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.
Для компонентов Start и End нужно применить forwardRef, либо другим образом соблюсти контракт для StrictMode. Иначе с тултипами вокруг них может быть проблема.
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.
Указал forwardRef
для Start и End
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.
Еще сам ref
нужно прокинуть, без этого не работает.
forwardRef((props, ref) => <Comp {...props} ref={ref} />)
Думаю тут тоже стоит сразу forwardRefAndName
использовать.
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.
Прокинул ref
и сделал forwardRefAndName Start/End в DateRangePickerInput.tsx
packages/react-ui/components/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
packages/react-ui/components/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
onValueChange={setStartValue} | ||
disabled={startDisabled} | ||
/> | ||
<DateRangePicker.Separator /> |
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.
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.
Хмм, здесь фрагмент с нативным DateInput для телефона.
Не могу воспроизвести баг. А можешь подробнее описать, как воспроизвести?
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.
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.
Починил — теперь нет лишней тирешки и теперь правильно отрабатывает фокус
optional?: boolean; | ||
onValueChange: (value: string) => void; | ||
} | ||
export type DateRangePickerFieldWithTypeProps = Omit<DateRangePickerFieldProps, 'type'>; |
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.
Тут наверное имелось в виду Without?
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.
Да, всё так, но уже неважно — я сделал проще и удалил этот фрагмент :)
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.
Там пока все еще остался DateRangePickerInputWithTypeProps
.
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.
Да, он-таки вернулся в forwardRefWithName, но теперь перестал экспортироваться)
useEffect(() => { | ||
if (isStart && startValue !== null) { | ||
props.onValueChange(startValue); | ||
} | ||
}, [startValue]); | ||
|
||
useEffect(() => { | ||
if (isEnd && endValue !== null) { | ||
props.onValueChange(endValue); | ||
} | ||
}, [endValue]); |
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.
А зачем и тут и в commonProps props.onValueChange
вызывать? Из-за этого он два раза вызывается при изменении значения.
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.
- Убрал дублирование из commonProps
- И докинул тестов
packages/react-ui/components/DateRangePicker/DateRangePickerField.tsx
Outdated
Show resolved
Hide resolved
6950e53
to
f3a399c
Compare
packages/react-ui/components/DateRangePicker/__tests__/getStateForValue-test.ts
Outdated
Show resolved
Hide resolved
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.
В свете последнего обсуждения, тесты на вызовы onValueChange как будто бы тоже были уместны.
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.
Да, я добавил несколько тестов с onValueChange и toHaveBeenCalledTimes:
- Проверка, что onValueChange не отрабатывает при монтировании (починил баг с тем, что мы тригерили onValueChange при каждом прокидывании в контекст)
- Проверки, что нет лишних вызовов при вводе в start/end
- Стирание значений
f3a399c
to
96e4794
Compare
children: React.ReactNode; | ||
} | ||
|
||
export const DateRangePicker = Object.assign( |
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.
Похоже, что тип у DateRangePicker
получился any
. И ему и его полям ts позволяет любые пропы передавать.
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.
dcc1454
to
ca4c46f
Compare
73e8461
to
d6aab5d
Compare
<DateRangePicker>
Привет! Сделал компонент для выбора периода
<DateRangePicker>
Документация | Песочница StackBlitz
<DateInput>
и одного общего<Calendar>
с диапазоном:<DateRangePicker.Start />
— начало периода, настройки как у<DateInput>
<DateRangePicker.End />
— конец периода, настройки как у<DateInput>
<DateRangePicker.Separator />
— разделитель<Calendar renderDay={...} />
. ПропrenderDay
так же доступен для настройки в DateRangePicker. Например, можно собрать календарь с ценами<DatePicker />
. Например,enableTodayLink
,useMobileNativeDatePicker
,onMonthChange
DatePicker
добавлен метод для валидацииDatePickerRange.validate(startValue, endValue, {options})
. В отдельном PR пример работы с библиотекой валидацийaria-group
иaria-label
для полей и правки в DateInputПредыдущее API для работы со значениями (устарело после обсуждений)
Работа со значениями
Важная особенность компонента — он не только состоит из нескольких частей, но и в нём сразу 2 input. В процессе разработки выработал 2 принципа:
value={['dd.mm.yyyy', 'dd.mm.yyyy']}
(формат настраивается)optional
,warning
,error
задаются в формате[true, false]
children
подразумевается только кастомизация оберток и<DateInput>
без работы со значениямиСсылки
IF-87 — там ссылки на дизайн, референсы и фидбек от разных команд
Чек-лист перед запросом ревью
Добавлены тесты на все изменения
✅ unit-тесты для логики
⌛️ скриншоты для верстки и кросс-браузерности
⬜ нерелевантно
Добавлена (обновлена) документация
✅ styleguidist для пропов и примеров использования компонентов
✅ jsdoc для утилит и хелперов
✅ комментарии для неочевидных мест в коде
✅ прочие инструкции (
README.md
,contributing.md
и др.)⬜ нерелевантно
Изменения корректно типизированы
✅ без использования
any
(см. PR2856
)⬜ нерелевантно
Прочее
✅ все тесты и линтеры на CI проходят
✅ в коде нет лишних изменений**
✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)