Task 135

[venglov]

[Ehevi]

Jak dla mnie wygląda i działa dobrze 👍
Mam kilka uwag, ale opis tego zadania na tarze jest dość ogólny i nie wiem, czy to też wchodziło w jego zakres. Jeśli nie, to dalszą część tego, co napisałam można uznać za czepianie się.

-> z Figmy chyba wynika, że przyciski 'Sprawdź plan' i 'Pokaż błędy' są połączone [ja rozumiem to tak, że naciśnięcie 'Sprawdź plan' go sprawdza i po sprawdzeniu otwiera okno z błędami, ale to może być zła interpretacja]
-> przycisk 'Pogląd' na Figmie ma etykietę 'Wyjdź'
-> w obecnej wersji jeżeli okno jest mniejsze, to przy scrollowaniu "tracimy" część zawartości, wyglądałoby lepiej gdyby tak nie było

-> dobrze byłoby sprawdzić zgodność względem ostatnich mergów do developa ;)

[Ehevi]

[Qwebeck]

Myślę, że lepiej tego nie robić: przyciski należą do elementów statycznych które zawsze są widoczne na stronie. Gdyby te przyciski zmieniały swoją pozycję to myślę, że to wyglądało by nieprzyjemnie.
Mi się wydaje, że jeździć mogą tylko elementy które nie są widoczne zawsze:
modale, drawery, snackbary, etc

[Qwebeck]

-> z Figmy chyba wynika, że przyciski 'Sprawdź plan' i 'Pokaż błędy' są połączone [ja rozumiem to tak, że naciśnięcie 'Sprawdź plan' go sprawdza i po sprawdzeniu otwiera okno z błędami, ale to może być zła interpretacja]

Uważam że to ma dużo sensu. Klikając Sprawdź plan na pewno chcesz zobaczyć te błędy co się pojawią.
Więc mi się wydaje, że moglibyśmy to podpiąć.

[Qwebeck]

Wygląda super, ale mam kilka drobnych uwag które wypisałem

[Qwebeck]

Ja bym zmienił nazwę interfejsu na RouteButtonOptions/Props, ponieważ tak naprawdę ten argument jest przyjmowany przez ten komponent

[Qwebeck]

Lepiej na Options, bo tak to jest zapisane w konwencji.
https://github.com/Project-Summer-AI-Lab-Glider/nurse-scheduling-problem-frontend/wiki/Struktura-projektu-i-konwencja-kodu#komponenty

Twoje podejście też nie jest źle, ale wolałbym nie używać różnych styli w aplikacji, ponieważ to wprowadza później zamieszanie + kod wygląda mniej czytelnie: w różnych miejscach różne nazwy dla tego samego.

BTW, to dodajemy przed Options nazwę komponentu żeby to potem można było rozszerzyć nowym interfejsem w wygodny sposób. Np.:

interface ShiftCellOptions extends BaseCellOptions {
...
}

[Qwebeck]

Moim zdaniem o wiele sensowniejszą nazwą byłoby disabled, a nie editMode, ponieważ:

1. Nie konieczenie musimy mieć editMode, żeby chcieć zablokować tab
2. Nie koniecznie mając editMode chcemy zablokować taby
3. To pole nie odpowiada za rzeczy związane z edycją, a tylko za te, co są związane z wlączeniem/wylączeniem przycisków.

[Qwebeck]

Pisałem o tym wyżej

[Qwebeck]

Ja bym zrobił to tak:

<ScheduleViewOnlyMode openEdit={() => props.editModeHandler(true)} />

To moim zdaniem jest lepsze ponieważ komponent viewOnly nie potrzebuje mieć możliwości otwierać lub zamykać tryb edycji.
Ponadto, uważam, że ten komponent nie powinien wiedzieć o tym, że żeby przejść do trybu edycji, on musi przekazać true do funkcji ediModeHandler.

[Qwebeck]

Można też nie używać klamr jeżeli przekazujemy strign

[kaplonpaulina]

+1
trzymajmy się jednej konwencji, w tym pliku użyłeś jednocześnie className={""} i className=""

[Ehevi]

Myślę, że lepiej tego nie robić: przyciski należą do elementów statycznych które zawsze są widoczne na stronie. Gdyby te przyciski zmieniały swoją pozycję to myślę, że to wyglądało by nieprzyjemnie.
Mi się wydaje, że jeździć mogą tylko elementy które nie są widoczne zawsze:
modale, drawery, snackbary, etc

Źle wyraziłam, o co mi chodzi. Chciałam zasugerować, żeby właśnie te elementy zawsze były widoczne na stronie bez względu na scrollowanie, np. position fixed albo sticky. Wtedy nie uciekałyby przy przewijaniu i użytkownik cały czas miałby do nich dostęp. Anyway, jestem świeżynką w temacie, więc statystycznie większość moich pomysłów nie będzie bardzo dobra XD
I warto mieć nadzieję, że użytkownik końcowy raczej nie będzie na siłę utrudniał sobie życia i będzie preferował pracę na pełnym oknie, bez konieczności scrollowania ;)

[Qwebeck]

Źle wyraziłam, o co mi chodzi. Chciałam zasugerować, żeby właśnie te elementy zawsze były widoczne na stronie bez względu na scrollowanie, np. position fixed albo sticky. Wtedy nie uciekałyby przy przewijaniu i użytkownik cały czas miałby do nich dostęp.

Myślę, że to czy muszą być statyczne czy nie, do przegadania z Agą.

Anyway, jestem świeżynką w temacie, więc statystycznie większość moich pomysłów nie będzie bardzo dobra XD

Ten akurat nie jest zły, a tylko źle zrozumiany ;)

I warto mieć nadzieję, że użytkownik końcowy raczej nie będzie na siłę utrudniał sobie życia i będzie preferował pracę na pełnym oknie, bez konieczności scrollowania ;)

Niestety będzie musiał skrolować, bo harmonogram jest duży i nie mieści się na jednej stronie.
Chyba że zrobimy tak, że skrolowalny będzie tylko kontener, który zawiera sekcje zmian).
Myślę, że to jest sensowne:

• zafiksowany nałówek u góry
• zafiksowana liczba dzieci u dołu
• skrolowalne zmiany.

Ale wtedy potrzeba będzie też zastanowić się nad tym, jak to współgra z tym, że chcieliśmy robić zwijalne sekcje ( mi się wydaje, że jeżeli będzie scroll, to w tym miejscu raczje się nie przydadzą )

[kaplonpaulina]

Ogólnie dobra robota, ale mam kilka uwag:

Suwak na dole strony powinien być niewidoczny kiedy jest nieaktywny:

Jak klikę Plik i wczytam plik i nie odkliknę to się robią śmieszne rzeczy jak scroluję góra-dół:

Suwak do tej pory działał tak:
Jak trzeba to się pojawiał jako element okna a nie strony, więc był zawsze widoczny kiedy mamy pomniejszone okno.

Teraz kiedy pomniejsze okno suwak jest na samym dole strony i żeby go użyć muszę zjechać na sam dół, uważam, że przedtem było lepiej i wygodniej:

Można zostawić wymuszony scroll po prawej bo jest zawsze tam gdzie być powinien(jest zawsze dostępny) i nie rzuca się w oczy tak jak ten zaraz pod harmonogramem.

Jeśli chodzi o problem pzresuwania i "kończenia się" toolbaru, zgadam się i według mnie trzeba zrobić fixed toolbar, znaczy, że nie ważne jak zesklorujemy toolbar nie będzie się zmieniał względem okna aplikacji.

Dodatkowo dodałabym też ograniczenia do samego elektrona odnośnie wielkościokana aplikacji -> w sesnie dodać minimalne wymiary okna, na które pozwalamy

[Qwebeck]

Jak klikę Plik i wczytam plik i nie odkliknę to się robią śmieszne rzeczy jak scroluję góra-dół:

Super uwaga!
Zrobiłem na to zadanie na Tarze

Teraz kiedy pomniejsze okno suwak jest na samym dole strony i żeby go użyć muszę zjechać na sam dół, uważam, że przedtem było lepiej i wygodniej:

Dodałem suwak taki jak ty powiedziałaś. Teraz jest elementem strony

Dodatkowo dodałabym też ograniczenia do samego elektrona odnośnie wielkościokana aplikacji -> w sesnie dodać minimalne wymiary okna, na które pozwalamy

Ustawiłem 1500x500

[Qwebeck]

Kończę te dyskusje :D

Super praca, super review: dużo cennych uwag który potrzeba będzie omówić