19: Adding lesson modal #78
Conversation
|
marina-bul
temporarily deployed
to
xi-development
GitHub Actions
Inactive
| import type { FC, PropsWithChildren } from 'react'; | ||
| import type { FormData } from '../../model'; | ||
|
|
||
| interface AddingFormProps extends PropsWithChildren { |
There was a problem hiding this comment.
тут можно сделать type
и если используется PropsWithChildren то ему в дженерик можно передать тип для расширения PropsWithChildren
|
Поправь, пожалуйста, вёрстку, кроме красных стрелок, ещё хотелось бы прижать кнопки действий в низу модалки 
|
unknownproperty
left a comment
unknownproperty
left a comment
There was a problem hiding this comment.
Марина, посмотри, пожалуйста, типизацию на предмет использования type и interface, у тебя где-то используется одно, где-то другое. Мы всё же пишем типы почти везде через type и хотелось бы соблюдать общей code style
| export const AssignLessonButton = ({ className }: { className?: string }) => { | ||
| type AssignLessonButtonProps = { | ||
| className?: string; | ||
| onButtonClick?: () => void; |
There was a problem hiding this comment.
Как будто слишком обобщённое название, хотелось видеть что-то конретное, либо просто базу handleClick
| <Button | ||
| size="s" | ||
| className="text-s-base text-brand-0 hidden rounded-lg px-4 sm:flex" | ||
| onClick={onButtonClick} |
There was a problem hiding this comment.
Функция может быть undefined, исходя из типизации, быть может стоит сделать проверку на её наличие. Не уверен, будут ли тут ошибки, если не передать, но лучше всё же подумать, как это поправить
There was a problem hiding this comment.
Лучше вместо utils.ts написать в названии файла то, что в нём содержится и что мы из него экспортируем getFullDateString.ts
There was a problem hiding this comment.
Возможно ли как-то иначе поправить проблему, например патчем самого компонента в xi.kit?
There was a problem hiding this comment.
если проблема в том, что не выбирается дата в календаре, то это из-за того что модальное окно при появлении навешивает pointer-events: none на body
соответственно, когда открывается календарь нужно снимать pointer-events
тут как вариант можно завернуть daypicker в поповер или исправить сам daypicker что бы он себя вел как modal (навешивал/снимал pointer-events)
| <Close className="fill-gray-80 sm:fill-gray-0 dark:fill-gray-100" /> | ||
| </ModalCloseButton> | ||
| <ModalContentWrapper currentDay={eventDate}> | ||
| <AddingForm onClose={handleCloseModal} onDateChange={setEventDate}> |
There was a problem hiding this comment.
Не совсем понимаю, зачем делать AddingForm как wrapper?
Если дело в submit, то можно использовать id для формы
| import { DatePicker } from '@xipkg/datepicker'; | ||
| import { Calendar } from '@xipkg/icons'; | ||
| import { Input } from '@xipkg/input'; | ||
| import { getFullDateString } from '../../utils/utils'; |
There was a problem hiding this comment.
Не очень красивый импорт и экспорт в utils, я бы добавил index
| useEffect(() => { | ||
| if (value) { | ||
| setDate(value); | ||
| } | ||
| }, [value]); |
There was a problem hiding this comment.
Тоже, что и коммент ранее, странно выглядит, для управления каким-то общим состоянием в разных компонентах лучше использовать небольшой Zustand store
| <DatePicker | ||
| calendarProps={{ mode: 'single', selected: date, onSelect: handleSelectDate, required: true }} | ||
| > | ||
| <Input |
There was a problem hiding this comment.
На самом деле можно вместо input тут использовать Button, так по факту само поле ввода нам не нужно, только надо выводить информацию и открывать сам DatePicker
| import { DayCalendar } from './DayCalendar'; | ||
| import type { FC, PropsWithChildren } from 'react'; | ||
|
|
||
| interface ModalContentWrapperProps extends PropsWithChildren { |
There was a problem hiding this comment.
Давай перепишем на type
| const handleClearForm = () => { | ||
| setValue('title', DEFAULT_VALUES.title); | ||
| setValue('description', DEFAULT_VALUES.description); | ||
| setValue('studentId', DEFAULT_VALUES.studentId); | ||
| setValue('startTime', DEFAULT_VALUES.startTime); | ||
| setValue('endTime', DEFAULT_VALUES.endTime); | ||
| setValue('startDate', DEFAULT_VALUES.startDate); | ||
| setValue('shouldRepeat', DEFAULT_VALUES.shouldRepeat); | ||
| }; |
There was a problem hiding this comment.
Мне не очень нравится повторение и вызов функции 7 раз подряд. А ты не смотрела на метод reset в react-hook-form?
No description provided.