Frontend review #3

[Arsenicro]

• [x] https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Navbar/Navbar.tsx#L9 Wiem że to pozostałość po jakiejś przeszłej implementacji, ale w tym momencie Navbar_AppBar to nie jest już do końca styles co sugeruje prefix nazwy. To już jest rzeczywisty komponent. W przyszłości może okazać się mylące, że plik nazwany styles zawiera coś co jest komponentem. Możliwe że chodziło o rozdzielenie odpowiedzialności i trzymanie stylów osobno, ale teraz i tak tego nie robicie: w tym pliku są wykorzystywane funkcje i komponenty które nie mają zbyt dużo wspólnego ze stylowaniem.
• [x] https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Navbar/Navbar.tsx#L14 Po co to przypisanie?
• [x] https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Navbar/Navbar.tsx#L22 To że tutaj jest funkcja zwracająca funkcję jest tylko dlatego, że potem chcecie tego użyć w evencie onClick, nic więcej nie sugeruje że wynikiem będzie funkcja. Lepszą nazwą pewnie byłoby getOnChangeSubSiteFunction czy coś podobnego - coś co już na etapie czytania tej funkcji sugeruje, co ona robi i do czego służy. changeSubSite sugeruje funkcję, która coś robi, a nie funkcję, która zwraca coś, co trzeba wywołać.
• [x] https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Navbar/Navbar.tsx#L32 To nie jest najlepszy pomysł by mieć obrazek z DC jako cokolwiek. Jeśli go potrzebujecie, wgrajcie go sobie do repo i używajcie go z lokalnego linku. Inaczej kiedyś DC wam wyrzuci/podmieni obrazek i przestanie działać bez waszej wiedzy, przez zewnętrznego dostawcę.
• [x] https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/page/offer-page/OffersPage.tsx#L29 Rozumiem że jeśli backend zwróci totalPages jako -1, 0 i 1 to stron jest zawsze 1? Bardziej adekwatnie byłoby powiedzieć że jeśli nie mamy przynajmniej jednej strony to mamy ich null i obsłużyć jakoś inaczej empty state, niż pokazywać że stron mamy jedną (ale to jest uwaga bardziej stylistyczna niż funkcjonalna).
• [x] https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/page/offer-page/OffersPage.tsx#L38 Czemu ten stan jest arrayką? Jak już to powinien być obiektem, a pewnie najlepiej rozbić to na dwa stany, połowa użyć tej funkcji to zmiana tylko strony i zachowanie ofert bez zmian.
• [ ] https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/page/offer-page/OffersPage.tsx#L38 To jest w ogóle cały czas omijanie wszystkich zalet jakie daje nam useQuery. Przeczytajcie dokładnie dokumentację. Po co to offers ustawiane za każdym razem w stanie, po co to onSuccess kilka linijek niżej, jak useQuery zwraca nam taki ekstra obiekt z wszysktimi danymi? Robicie

  async () => {
    const res = await OfferService.findAll(page);
    return {
      offers: mapOffers(res),
      page: mapPage(res),
    };
  }

  zamiast linijki 46, const { data } = useQuery( zamiast 44 i w obieckie data macie wszystko czego potrzebujecie w formacie jaki potrzebujecie i jeszcze useQuery dba o cache. Page wyświetlacie z backendu, a ta trzymana w stanie jest tylko po to żeby wiedzieć o jaką requestowaliście useQuery.

• [x] https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/page/offer-page/OffersPage.tsx#L41 A co ja mówiłem o ustawianiu stanu jeśli nowy stan zależy od poprzedniego stanu? Tutaj powinna być funkcja setOffers(([currentOffers]) => [currentOffers, _page])
• [ ] https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/page/offer-page/OffersPage.tsx#L52 To nie jest najlepsza technika obsługi błędów. Jeśli błąd wynikna z niedziałającego backendu (a nie np. z braku strony o którą pytacie), to w tym momencie jedyne co wiecie to że macie pustą tablicę i domyślną stronę. Jeśli ktoś był na stronie 5, na stronie 6 dostał błąd, to nie zostanie o nim powiadomiony, nie zobaczy komunikatu o błędzie, a przekieruje go na stronę pierwszą i wyczyści mu tablicę. Co więcej, przez to co pisałem wyżej, pewnie nawet nie wczyta mu się strona pierwsza. Lepiej pokazać jakiś błąd i wtedy zrobić jakiś sensowny handling (może to być cofnięcie strony, może to być pójście na pierwszą, ważne żeby to było spójne).
• [ ] https://github.com/Lerqiu/FOODE/blob/bc3c64cd9f7e9b3d7ea449d16b4488025a6a3825/frontend/src/components/Offers/OfferSidebar/OfferSidebar.tsx#L57 do useQuery te same uwagi co wyżej, dodatkowo to o czym mówiłem na wykładzie, nie invalidujcie tych query ręcznie, to się absolutnie mija z celem, rzeczy które useQuery używa do wykonania requesta powinny być w tablicy zależności (powinny być elementami klucza tablicy)
• [ ] https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Offers/OfferSidebar/OfferSort/OfferSortSelect/OfferSortSelect.tsx#L8 Tak się raczej nie robi, bo uzależniacie się od formatu. To powinien być obiekt z propkami na pole i kierunek sortowania, a dopiero gdzieś dalej, tam gdzie z tego korzystacie, powinno być zmapowanie tego do jakiegoś napisu zrozumiałego przez to z czego korzystacie
• [ ] Ogólnie z tym filtrowaniem i sortowaniem macie taki problem że uparliście się żeby to trzymać w localstorage. To nie jest do końca złe podejście, ale nie jest też całkowicie dobre. Powinniście się skupić na zrobieniu tego tak żeby działało dobrze i było ładne nie uzależniając się od technologii, a dopiero później, ewentualnie, dodać funkcjonalność zapamiętywania stanu filtrów która mogłąby działać jakoś (może localStorage, może cookies, może globalny stan?). W szczególności, callowanie localStorage za każdym razem żeby wykonać request jest dość obciążające, to się powinno dziać leniwie (przy załadowaniu komponentu załaduj jego stan z localstorage, przy zmianie stanu zapisz go do localstorage, poza tym useQuery nie powinno wiedzieć o istnieniu localStorage)
• [x] https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/components/Offers/OfferSidebar/OfferFilter/OfferFilterText/OfferFilterText.tsx#L6 https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unused-vars.md
• [x] https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Offers/OfferSidebar/OfferFilter/OfferFilterCitySelect/OfferFilterCitySelect.styles.ts#L4 i https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Offers/OfferSidebar/OfferSort/OfferSortSelect/OfferSortSelect.style.ts#L4 są indentyczne, po co to powtórzenie? Może lepiej użyć theme do stylowania?
• [x] https://github.com/Lerqiu/FOODE/blob/2fbbe5ac932480be58865f83e4b34c94d025d855/frontend/src/services/CityService.tsx#L4 trzy razy tworzycie tego axiosa
• [x] https://github.com/Lerqiu/FOODE/blob/bc3c64cd9f7e9b3d7ea449d16b4488025a6a3825/frontend/src/components/Offers/AddOfferModal/AddOfferModal.tsx#L49 Co chwila widzę jakieś any...
• [ ] https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/components/Offers/AddOfferForm/AddOfferForm.tsx#L99 Czemu nagle rezygnujecie z materialowego selecta?
• [x] https://github.com/Lerqiu/FOODE/blob/b4a55d4e3a5551e845ca81128b7ec3ba1b3e8309/frontend/src/components/Offers/Pagination/ProductsOffersPagination.tsx#L19 Co tu się dzieje?
• [x] https://github.com/Lerqiu/FOODE/blob/bc3c64cd9f7e9b3d7ea449d16b4488025a6a3825/frontend/src/components/Offers/AddOfferModal/AddOfferModal.tsx#L32 Wynieście to nad komponent, żeby niepotrzebnie się nie zmieniało
• [x] https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Offers/OfferList/Offer/OfferDate/OfferDate.tsx#L7 A po co właściwie ta funkcja?
• [x] https://github.com/Lerqiu/FOODE/blob/8c5692f7dec3f330a4cacc65eb045629e5a380d2/frontend/src/App.test.tsx#L8 Lepiej wyrzucić te testy jeśli mają nie przechodzić, Janusz ucieszy się że macie jakieś testy a nie macie

[Lerqiu]

https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Navbar/Navbar.tsx#L14

Występuje wyłącznie dla wygody w definiowaniu ścieżki. Zmieniłem nazwę importu, by element stał się czytelniejszy.

[Lerqiu]

https://github.com/Lerqiu/FOODE/blob/b4a55d4e3a5551e845ca81128b7ec3ba1b3e8309/frontend/src/components/Offers/Pagination/ProductsOffersPagination.tsx#L19 Ograniczamy tu maksymalną liczbę wyborów stron na pasku paginacji. Poprawiłem nazewnictwo.

[Lerqiu]

https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/page/offer-page/OffersPage.tsx#L29

Rozumiem że jeśli backend zwróci totalPages jako -1, 0 i 1 to stron jest zawsze 1? Bardziej adekwatnie byłoby powiedzieć że jeśli nie mamy przynajmniej jednej strony to mamy ich null i obsłużyć jakoś inaczej empty state, niż pokazywać że stron mamy jedną (ale to jest uwaga bardziej stylistyczna niż funkcjonalna).

Z serwera otrzymujemy informację o ilości stron składający się na rekord (0 dla pustego zbioru) oraz komponent material UI wymaga liczby (dobrze prezentuje się dla >= 1). Ponieważ przez cały czas życia dysponujemy liczbą, stwierdziłem, że to będzie łatwiejsze/ładniejsze.

[Lerqiu]

https://github.com/Lerqiu/FOODE/blob/8317301ee828264016b243778763c8240935c902/frontend/src/components/Offers/OfferList/Offer/OfferDate/OfferDate.tsx#L7

To rzeczywiście nie musi być funkcją.

[Arsenicro]

https://github.com/Lerqiu/FOODE/blob/b4a55d4e3a5551e845ca81128b7ec3ba1b3e8309/frontend/src/components/Offers/Pagination/ProductsOffersPagination.tsx#L19

Ograniczamy tu maksymalną liczbę wyborów stron na pasku paginacji. Poprawiłem nazewnictwo.

Nadal nie rozumiem po co to jest. Jeśli pageCount nie może być większy niż 10 (nie wiem czemu tak miałoby być, co jeśli będzie więcej rekordów?), to nie powinno być opcji ustawienia tego na coś większego niż 10. W tym momencie pageCount może rosnąć do dużych liczb, ale liczba stron nadal będzie maksymalnie 10, co jest bardzo nieintuicyjne

https://github.com/Lerqiu/FOODE/blob/070ecbf7d30fc22276fc698a6819e767caa55f48/frontend/src/page/offer-page/OffersPage.tsx#L29

Rozumiem że jeśli backend zwróci totalPages jako -1, 0 i 1 to stron jest zawsze 1? Bardziej adekwatnie byłoby powiedzieć że jeśli nie mamy przynajmniej jednej strony to mamy ich null i obsłużyć jakoś inaczej empty state, niż pokazywać że stron mamy jedną (ale to jest uwaga bardziej stylistyczna niż funkcjonalna).

Z serwera otrzymujemy informację o ilości stron składający się na rekord (0 dla pustego zbioru) oraz komponent material UI wymaga liczby (dobrze prezentuje się dla >= 1). Ponieważ przez cały czas życia dysponujemy liczbą, stwierdziłem, że to będzie łatwiejsze/ładniejsze.

Tak, ale gubicie tą informację cemu tu jest 0. Jeśli backend zwrócił 0 albo -1 to znaczy że mamy jakąś niestandardową sytuację, coś poszło nie tak. Ustawienie tego na null ma więcej sensu. Tak, musimy wtedy zadbać przy renderowaniu o to, żeby to była odpowiednia liczba, ale wtedy mamy więcej opcji bez konieczności robienia refactoru. Możemy np. nie wyświetlać paginacji wcale, albo wyświetlić jakiś specjalny "pusty stan" paginacji. W tym momencie gubicie informację że coś poszło nie tak, na rzecz tego żeby gdzieś dalej, w jakimś potomku tego komponentu, napisać o jedną linijkę mniej. Takie kombinacje, jeśli już się dzieją, powinny się dziać w komponencie który ich potrzebuje (w tym przypadku komponent Paginacji), bo wtedy za dwa miesiące jak usiądziesz do kodu będziesz mógł łatwo coś zmienić (np jeśli za dwa miesiące ktoś ci każe nie pokazywać paginacji jeśli był błąd)