fix(Calendar): revert period #3415
Conversation
packages/react-ui/components/Calendar/__stories__/Calendar.stories.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| export const fromString = (dateString: string): CalendarDateShape => { | ||
| const [date, month, year] = dateString.split('.').map(Number); | ||
| return create(date, getMonthInNativeFormat(month), year); |
There was a problem hiding this comment.
Думаю, изначально было правильно - без getMonthInNativeFormat().
В нашем строковом представлении даты месяц начинается с 01. Это видно по старым примерам для Календаря, ДатаПикера и ДатаИнпута.
В примере в доке можно сразу создавать объект вида { year, month, date }, ведь CalendarDay предполагается использовать внутри renderDay, а там всегда приходит такой объект.
Если хелпер полезен для сторисов, то лучше использовать InternalDate, который уже умеет парсить и трансформировать дату.
new InternalDate({ value: dateString }).getComponentsLikeNumber()There was a problem hiding this comment.
Переосознал, переделал и подрезюмировал ниже механизмы работы с датами и форматами.
| const ariaLabel = `${locale.dayCellChooseDateAriaLabel}: ${new InternalDate({ langCode }) | ||
| .setComponents({ ...date }, true) | ||
| .toA11YFormat()}`; |
There was a problem hiding this comment.
Здесь date должен приходить с "человеческим" представлением месяца, где январь 1.
Так мы сохраним консистентность публичной работы с датами в наших контролах.
There was a problem hiding this comment.
В итоге ariaLabel и так получался в человеческом формате благодаря второму аргументу в setComponents.
Но в любом случае, я перевел CalendarDay целиком на работу с человеческим форматом.
There was a problem hiding this comment.
Да, тейк был именно в том, что приходится использовать второй аргумент, т.к. в пропах пользователь вынужден будет передавать нативный месяц.
| export const toString = ({ date, month, year }: CalendarDateShape): string => { | ||
| const [d, m, y] = [date, getMonthInHumanFormat(month), year].map((x) => x.toString()); | ||
| return `${d.padStart(2, '0')}.${m.padStart(2, '0')}.${y.padStart(4, '0')}`; | ||
| }; |
There was a problem hiding this comment.
Этот хелпер тоже повторяет возможности InternalDate
new InternalDate().setComponents({ date, month, year }).toString({ withPad: true })There was a problem hiding this comment.
Отписал ниже свои предложения по использованию CalendarDateShape и InternalDate
|
По поводу CalendarDateShape и InternalDate отпишу тут, в одном месте. У нас наблюдаются два пересекающийхся по функционалу механизма для работы с датами. Это CalendarDateShape и InternalDate. Я не в курсе всей истории и замыслов их создания. Но по косвенным признакам могу предположить следующее. InternalDate - это внутренний подкапотный механизм. Он содержит кучу логики, о которой пользователю знать нет нужды. И вроде бы он нигде не торчит наружу. CalendarDateShape - это публичный механизм. Он явным образом экспортируется из календаря. Он доступен пользователю и ограничен теми функциями, которые могут ему реально понадобиться для манипуляции с датами там, где строки не удобны. Например, для работы с периодами. Поэтому предлагаю считать CalendarDateShape публичным API, описать его в доке и использовать в примерах только его. InternalDate лучше оставить только для внутренних нужд компонентов и, когда-нибудь, навести в нем порядок. |
|
По поводу формата месяцев. Их тоже в компонентах наблюдается два: человеческий и нативный (начинающий отсчет с 0). До сих пор все даты, которые передавал или получал пользователь из контрола, мы старались предлагать исключительно в человеческом формате. Нативный же используется во внутрянке. В этом ПР появляется еще одно место соприкосновения пользователя с датой. Это проп date в CalendaDay. Изначально я не сильно раздумывал и оставил его в нативном формате. Но после ревью и составления всей картины выше стала очевидна непоследовательность этого API для пользователя. В итоге, предлагаю CalendaDay, как публичный компонент, тоже перевести на человеческий формат. Это позволяет также совсем избавиться от работы с форматами внутри CalendarDateShape и не заставлять пользователя вообще знать про нативный формат. |
|
Эти хелперы экспортируется из корня Календаря, и после его опубличивания они тоже стали публичными: import { isEqual, comparator } from '@skbkontur/react-ui';В своё время мы отбились от вопросов "почему строка, а не Moment/Date", и теперь могут возникнут вопросы "почему объект, а не Moment/Date". В идеале будет отрефачить и сам Если в Т.е. я бы предложил проп |
Проблема
В #3258 были добавлены пропы для периода дат и проведен некоторый рефакторинг.
Пропы для периода планируется использовать только внутри отдельного компонента PeriodPicker, но они значительно усложняют устройство самого Calendar. PeriodPicker может реализовать ту же функциональность внутри себя самостоятельно, используя более общий проп renderDay. Это не задача самого календаря. Он может оставаться простым. Поэтому считаю, что пропы для периода в Calendar не нужны. Их стоит откатить.
Проведенная в #3258 замена пропа onMonthChange на onStuckMonth и onMonthSelect кажется нецелесообразной. onMonthChange решает ту же задачу, но является более общим. Он не зависит от реализации смены месяцев (с залипанием или без). Возможно, его стоит переименовать на onVisibleMonthChange для большей интуитивности.
В #3258 также был добавлен debounce на вызов onStuckMonth. Его тоже предлагаю откатить. Во-первых, вызовов происходит не очень много. Debounce уменьшает количество вызовов на единицы. Что не очень существенно (но добавляет немного сложности). Во-вторых, не факт, что пользователю не будут интересны все вызовы. Среди них нет лишних или дублирующих. Они все отражают факт смены месяца. Так что, думаю, их не стоит debounce-ить.
Решение
Откачено
Переделано
Добавлено новое
Переделано дополнительно
Удалено дополнительно
Ссылки
IF-1702
Чек-лист перед запросом ревью
Добавлены тесты на все изменения
✅ unit-тесты для логики
✅ скриншоты для верстки и кросс-браузерности
⬜ нерелевантно
Добавлена (обновлена) документация
✅ styleguidist для пропов и примеров использования компонентов
⬜ jsdoc для утилит и хелперов
⬜ комментарии для неочевидных мест в коде
⬜ прочие инструкции (
README.md,contributing.mdи др.)⬜ нерелевантно
Изменения корректно типизированы
✅ без использования
any(см. PR2856)⬜ нерелевантно
Прочее
✅ все тесты и линтеры на CI проходят
✅ в коде нет лишних изменений
✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)