Poprawki strona projektu#60
Conversation
mtrznadel24
left a comment
mtrznadel24
left a comment
There was a problem hiding this comment.
Wygląda dobrze, kilka uwag do przemyślenia.
| UUID id, | ||
| LocalDate startDate, | ||
| LocalDate endDate, | ||
| AssignmentStatusResponse status, |
There was a problem hiding this comment.
Co prawda to nie powstało w tym PR chyba, ale w sumie po co to AssignmentStatusResponse. AssignmentStatus to tylko enum i stworzenie response nie daje nam tu żadnych korzyści, a nawet przeszkadza, bo wymaga dodatkowego mapowania, zwróciłbym po prostu AssignmentStatus.
There was a problem hiding this comment.
Tak, to nie powstało w ramach tego taska, skorzystałem z istniejącej struktury. Jestem zdania, że zawsze powinno się rozgraniczać te warstwy logiki, tzn AssignmentStatus mimo, że wygląda identycznie to on dotyczy encji bazodanowych. Więc tak samo jak nie korzystamy z encji, to dla enuma również robimy osobne mapowanie, a szczególnie jest to zauważalne w momencie kiedy enum bazodanoway zaczyna rozjeżdżać się z tym co chcemy przekazać na frontend, np. zobacz różnicę między UserRole oraz AdminAssignableRole to drugie nie ma w sobie admina.
There was a problem hiding this comment.
No tak ale AdminAssignableRole to enum, który do czegoś służy dodatkowo, czyli w tym wypadku nie chcemy zwracać Administrator, a tamto response to po prostu zduplikowany enum. Według mnie trochę przerost formy nad treścią, ale okej można zostawić.
| "WHERE pa.project.id = :projectId " + | ||
| "AND pa.status IN ('PENDING', 'ACCEPTED') " + | ||
| "ORDER BY pa.createdAt ASC") | ||
| List<ProjectAssignment> findByProjectIdOrderByCreatedAtAsc(@Param("projectId") UUID projectId); |
There was a problem hiding this comment.
Troszke myląca nazwa, bo sugeruje zwracanie wszystkich assignmentów.
Proponuje findActiveAndPendingByProjectIdOrderByCreatedAtAsc
| const calculatePositionAndWidth = (start: Date, end: Date, projectStart: Date, projectEnd: Date) => { | ||
| const totalProjectDuration = projectEnd.getTime() - projectStart.getTime(); | ||
| const taskDuration = end.getTime() - start.getTime(); | ||
| const timeOffset = start.getTime() - projectStart.getTime(); |
There was a problem hiding this comment.
Wygląda na to, że sam to napisałem xD, ale można dodać +1 (czyli właściwie + 86400000, bo to milisekundy) do totalProjectDuration i taskDuration bo np jak rozpocznie sie 12, a zakończy 14 to 14 - 12 = 2, a realnie to są 3 dni, bo 14 też wliczamy.
| <div | ||
| className="absolute top-0 -translate-x-1/2 cursor-pointer group" | ||
| style={{ left: position.left }} | ||
| > |
There was a problem hiding this comment.
można dodać minimalny padding na diva p-1 albo nawet p-2, będzie się łatwiej klikało, bo MapPin jest bardzo mały.
There was a problem hiding this comment.
zmiana paddingu chyba nic nie daje, zmieniłem ręcznie na 8x8 rozmiar ikonki
2 participants
zmieniłem harmonogram na stronie projektu, tak żeby nie używał już zhardcodowanych danych, wyświetlany jest też stan wniosku