Conversation
* Added @faker-js/faker as a dependency in package.json and package-lock.json * Introduced type definitions for the faker module in src/types/faker.d.ts
…ties * Updated MailHistoryPopup to use responsive dimensions. * Introduced EmailHistoryToolbar for global and status filtering. * Implemented sorting functionality in the email history table headers. * Created useEmailHistoryTable hook to manage table state and filters. * Added mock data generation for email history using @faker-js/faker.
|
The preview deployment for Eventownik Frontend Preview is ready. 🟢 Open Preview | Open Build Logs | Open Application Logs Last updated at: 2026-03-01 16:47:42 CET |
|
Closes #312 |
* Deleted @faker-js/faker from package.json and package-lock.json. * Removed email history mock generation logic from the template-entry component and deleted the associated mock file.
There was a problem hiding this comment.
Na plus
Porządny kod z samą tabelą
Do dorobienia dodatkowo
- Tłumaczenia wszystkich labelów na angielski - też możesz zerknąć w kod zrefaktorowanej tabeli uczestników żeby zobaczyć jak się to robi
- Przydałaby się jeszcze liczba wszystkich wierszy w tabeli. Imo gdzieś obok samego search bara albo pod samymi wierszami sticky napis (tam gdzie normalnie byłaby paginacja której tu imo nie trzeba dorabiać)
Uwagi ogólne poza komentarzami
- Responsywność dialogu zdecydowanie do poprawy - zawsze sprawdzaj responsywność przed daniem do review
- Tak jak pisałem w poprzednim review, "Closes #id issue" wpisuje się w opis pull requesta a nie w komentarz bo inaczej nie działa, alternatywnie też można oznaczyć które issue zamyka ten pr przez sidebar
- Search engine to jest google a nie input z filtrem
| }} | ||
| /> | ||
|
|
||
| <ScrollArea className="max-h-[60vh] min-w-0 flex-1"> |
There was a problem hiding this comment.
Te dwie klasy nic tu nie robią
| <ScrollArea className="max-h-[60vh] min-w-0 flex-1"> | |
| <ScrollArea className="max-h-[60vh]"> |
| </Button> | ||
| </DialogTrigger> | ||
| <DialogContent className="max-h-96 max-w-128"> | ||
| <DialogContent className="max-h-[80vh] max-w-[80vw]"> |
There was a problem hiding this comment.
vw nie jest odpowiednią jednostką do layoutów - jak już to zazwyczaj korzysta się z niej przy responsywnych font size'ach z clamp() - powiedziałbym że użycie jej tutaj to główny powód dla nieresponsywności dialogu. Ma on ciągle tylko maksymalnie 80% szerokości viewportu więc na telefonie ma bardzo bardzo mało miejsca. Dobry film wyjaśniający dlaczego nie powinno się korzystać z viewportowych jednostek w ten sposób: https://youtu.be/veEqYQlfNx8?t=449. vh jest już częstsze przy layoutach - najczęściej przy min-h-screen => min-height: 100vh ale z tego samego powodu co vw, działa słabo na telefonach. W przypadku dialogów multistepów mamy sm:max-h-[90vh] więc operujemy na vh tylko na ekranach szerszych niż 640px więc działa już to lepiej (plus 90 to więcej niż 80).
Cały dialog jest nieresponsywny. Zerknij w to jak są zrobione dialogi od multistepów (np. dialog tworzenia nowego wydarzenia). Ten dialog, z historią wysyłek, powinien tak samo wypełniać na telefonie cały ekran jak właśnie wszystkie multistepy. W trakcie robienia tego upewnij się też że sama tabela nie overflowuje i nie ucieka z całego dialogu (tak jak jest teraz) - choć nie jest ona w dialogu to mimo swojej szerokości tablica listy uczestników też nie overflowuje więc też mógłbyś na nią zerknąć.
| {targetMail !== null && <MailHistoryPopup email={targetMail} />} | ||
| {emailForHistory !== null && ( | ||
| <MailHistoryPopup email={emailForHistory} /> | ||
| )} |
There was a problem hiding this comment.
To trzeba jakoś przerobić. Z taką implementacją (którą ja robiłem, sory XD) po wejściu na stronę z listą maili:
- fetchuje się cała lista emaili dla wydarzenia (array typu
EventMail[]zgetEventEmails, endpoint.../{eventId}/emails), - dla każdego maila wywoływany jest dodatkowy fetch z endpointu na pojedynczy email (typ
SingleEventEmailzgetSingleEventEmail, endpoint.../{eventId}/emails/{emailId}).
Gdy zostanie wdrożony nowy edytor maili będzie to robiło całkiem ogromny niepotrzebny payload z serwera. Na SingleEventEmail jest to czego nie ma na EventEmail:
contentczyli HTML emaila,participantsczyli historia wysyłki, a dokładniej array uczestników którzy otrzymali tego maila wraz zmetagdzie są dane wysyłki, więc dokładnie to co jest wykorzystywane w tabeli,- a po wdrożeniu nowego edytora dodatkowo
schemaczyli struktura całego dokumentu z blokami do edytora (jebitny JSON).
Założeniem endpointu dla wszystkich emaili jest to, że właśnie nie zwraca tych ogromnych stringów bo nie są potrzebne w samej liście, tylko przy edycji. Aktualna implementacja fetchuje zarówno wszystkie maile jak i dodatkowe dane dla każdego z maili, totalnie psując to założenie.
- Postaraj się wymyśleć sposób w jaki można by to było ogarnąć bez ingerencji backendu - na teraz myślę że najprościej byłoby robić fetch po
SingleEventEmailw samej tabeli z historią przez useQuery na wzór tego jak jest to robione w zrefaktorowanej tabeli uczestników (participants-loader.tsx), ale byłoby fajnie gdyby się udało robić to serversideowo. - Jak się nie uda to poprosimy backend aby na endpoincie ze wszystkimi mailami było też info o historii wysyłki. Wciąż zwiększy to payload no ale bez content i schema przynajmniej.
| <SelectGroup> | ||
| <SelectLabel>Status</SelectLabel> | ||
| <SelectItem value="all">Wszystkie</SelectItem> | ||
| <SelectItem value="sent">Wysłane</SelectItem> |
There was a problem hiding this comment.
Żeby było spójnie z tym co w columns.tsx
| <SelectItem value="sent">Wysłane</SelectItem> | |
| <SelectItem value="sent">Wysłano</SelectItem> |
maciejkrol18
changed the title
|
Czy filtrowanie nie lepiej by było zrobić tak jak w liście uczestników żeby było spójnie? Czyli ikonka przed nazwą kolumny Ewentualnie może jakoś oznaczyć ten aktualny dropdown, że pozwala na wybór statusu, bo dopóki go nie rozwinęłam to nie domyśliłam się do czego jest |
|
A przy okazji testowania sobie znalazłam ciekawy błąd, który by można było od razu rozwiązać. I przy okazji v2 - kiedy pokazuje się "Nieudane"? Bo u mnie wszędzie gdzie się nie udało to pokazuje się "Oczekujące" |
* Removed unnecessary min-width style from ScrollArea for better layout consistency.
…nality * Introduced translations for the EmailHistoryTable in both English and Polish. * Updated EmailHistoryToolbar to utilize translations for placeholders, labels, and tooltips, enhancing localization support.
* Updated the Polish translation for 'sent' from 'Wysłane' to 'Wysłano' for improved accuracy.
…r experience * Updated MailHistoryPopup to include responsive height and display the number of recipients. * Improved EmailHistoryToolbar by adjusting input styling for better readability.
* Removed unused type reference for SingleEventEmail in template-entry component. * Updated date and time formatting in email columns to handle default date case, returning "-" for "01.01.1970".
* Updated date and time formatting logic in email columns to return "-" for "01.01.1970" only when the email status is "pending". This improves clarity in the email history display.
…slations * Added a status filter button to the EmailHistoryTable for improved filtering of email statuses. * Introduced new translations for the filter menu and recipients count in both English and Polish. * Updated EmailHistoryToolbar and MailHistoryPopup to reflect changes in filtering functionality and display recipient count.
|
Zostało mi ogarnięcie fetchowania |
3 participants
No description provided.