CRUD dla Wydarzeń - close #646 #644 #645 #646 #723
Conversation
|
Po przejściu na https://small-eod-git-fork-michalkarol-feature-events-646-644-645-646.watchdogpolska.vercel.app/events/new otrzymuje w przeglądarce biały ekran, a w konsoli: |
|
@ad-m: Poprawione. Już śmiga jak należy. |
|
Po przejściu do dodawania nowej sprawy podświetlają się obie pozycje menu:
Jaki mamy plan na datepicker w polu "Data"? Innych uwag nie mam. |
|
PR się zrobił konfliktowy. |
|
Poprawię jutro :D |
MichalKarol
force-pushed
the
feature/events-646-644-645-646
branch
from
cc01d04
to
c11050d
Compare
|
@ad-m: Poprawione. Niestety musiałem dodać redirect, bo wyjątkowo ignorowało exact w routach. |
|
|
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
| const editedEvent = events.data.find(value => value.id === Number(match.params.id)); | ||
| const [form] = Form.useForm(); | ||
| function onRequestDone(response: ServiceResponse<Event>) { |
There was a problem hiding this comment.
Robi się trochę WET: funkcje onRequestDone i onFinish są kopiowane-wklejane z komponentu na komponent z paroma tylko modyfikacjami (router.push({ where }), localeKeys.cases.{ whichView }).
Warto zrobić w tym momencie refactor, jakoś sparametryzować, wyciągnąć do osobnego pliku, tak, żeby widok mógł ich używać jakoś tak:
const onRequestDone = onRequestDoneFactory<Event>(form, '/cases', 'detailView');
const onFinish = onFinishFactory('cases', onRequestDone);
Czy coś w tym guście.
There was a problem hiding this comment.
Zarówno onRequestDone jak i onFinish zależą w dużej mierze od kontekstu (onFInish: nazwa resourca, stan isSubitting, dispatch, form i opcjonalny resoureId, onRequestDone: nazwa resource, form, router, locale, stan isSubmitting). Wyciąganie tego do generyków spowoduje że będziemy mieli 2x funkcje która przyjmuje 5 albo jedną funkcję która przyjmuje 7 parametrów. Troszeczkę popracowałem z localami żeby były bardziej zwięzłe, ale wydaje mi się że to są zbyt zależne od kontekstu funkcje żeby je przenosić do generyków.
There was a problem hiding this comment.
To nie wymaga aż tak znowu wielu parametrów: np. router to zwykły import. Dla onRequestDone będzie tego cztery parametry (setIsSubmitting, form, 'localeKeys.users', isEdit), z onFinish jest istotnie gorzej. Można zacząć od onRequestDone, onFinish odkładając na następny raz.
Natomiast takie kopiuj-wklej skutkuje kłopotami w przyszłości (gdy np. będzie potrzeba zmiany zachowania aplikacji - trzeba będzie tę samą zmianę pieczołowicie wydłubać w każdym komponencie), i prowokuje bugi (np. teraz komunikat błędu w UsersListView to "Nie udało się usunąć sprawy" - formatMessage({ id: localeKeys.cases.list.failedRemove }) - a nie "...usunąć użytkownika"). Im większy kawał kodu kopiujesz-wklejasz-zmieniasz tylko istotne kawałki, tym łatwiej takie błędy powstają.
There was a problem hiding this comment.
@fervero Zmiany do wszystkich widoków wrzucę w osobnym PR, bo obecnie znacząco rozrósł by się. Będą tam zmiany na paginacje, onRequestDone, onFinish i onDelete.
| router.push(`/events/edit/${id}`); | ||
| } | ||
|
|
||
| function onRemove(id: number) { |
There was a problem hiding this comment.
Analogiczna uwaga jak do onRequestDone i onFinish: wyekstrahować kawał wspólny dla wszystkich ListView, i tu zostawić jakiś
const onRemove = onRemoveFactory('cases/remove', 'cases');
There was a problem hiding this comment.
Nie chcę przenosić logiki pokazywania błędu do fabryk czy wrapperów na funkcję. Równie dobrze można to przenieść do serwisów reduxowych, ale nie wiem czy chcemy w to iść.
| }); | ||
| } | ||
|
|
||
| async function fetchPage(props: PaginationParams): Promise<PaginationResponse<Event>> { |
There was a problem hiding this comment.
Sytuacja podobna jak z onRemove.
There was a problem hiding this comment.
Odpowiedź taka sama jak na onRemove
| }, | ||
| }, | ||
| detailView: { | ||
| newPageHeaderContent: 'Tworzenie wydarzenie', |
MichalKarol
force-pushed
the
feature/events-646-644-645-646
branch
from
6ef33a7
to
dedb826
Compare
|
Mówisz o tym przycisku? Ja już go kliknąłem... |
|
Na cudzą odpowiedzialność mergowanie - proszę bardzo 😉 |
CRUD dla Wydarzeń.
Zamyka #646 #644 #645 #646