#699 Wyswietlanie dni wolnych w kalendarzu #1627
Conversation
|
Nie wiem, czy o tym rozmawialiśmy wcześniej, ale niespecjalnie podoba mi się, że w widoku miesiąca wykropkowane podkreślenie dostają również godziny i nazwy wydarzeń: Pokrewna obserwacja jest jeszcze taka, że mamy dwie pary funkcji o bardzo podobnej logice i różniące się tylko wykorzystywanym endpointem z jednej, oraz nadawanymi klasami i wartością atrybutu |
zapisy/apps/schedule/views.py
Outdated
| except Exception: | ||
| raise Http404 |
There was a problem hiding this comment.
Po pierwsze nie powinniśmy "blankietowo" łapać wyjątków – na pewno możemy tu wymienić KeyError, a jeśli chodzi o ew. brak atrybutu GET obiektu request to można to w ogóle ogarnąć inaczej, np. dekoratorem require_GET (o ile w ogóle ten atrybut bywa nieobecny nawet przy żądaniach POST, nie pamiętam w tej chwili).
Po drugie – czy 404 to najwłaściwszy kod HTTP? Zasób istnieje, tylko ktoś próbuje z nim niepoprawnie rozmawiać.
zapisy/apps/schedule/views.py
Outdated
| end = datetime.datetime.strptime(end, input_date_format) | ||
| formatted_start = start.strftime(output_date_format) | ||
| formatted_end = end.strftime(output_date_format) | ||
| response = freedays.filter(day__gte=formatted_start, day__lte=formatted_end).values() |
There was a problem hiding this comment.
Django i tak zapewne dopiero tutaj wykona zapytanie do bazy, ale gdyby zrobić
| response = freedays.filter(day__gte=formatted_start, day__lte=formatted_end).values() | |
| response = Freeday.objects.filter(day__gte=formatted_start, day__lte=formatted_end).values() |
i oczywiście wyrzucić wcześniejsze odwołanie do Freeday.objects, to stałoby się to jawne.
zapisy/apps/schedule/views.py
Outdated
| Both dates has to be given in 'input_date_format'. Date is returned in 'output_date_format'. | ||
| Returns 404 if one or both paramas are missing. |
There was a problem hiding this comment.
| Both dates has to be given in 'input_date_format'. Date is returned in 'output_date_format'. | |
| Returns 404 if one or both paramas are missing. | |
| Both dates have to be given in 'input_date_format'. Date is returned in 'output_date_format'. | |
| Responds with 404 if one or both params are missing. |
A co do tej "zwracanej daty" – czy to jest prawda? Data, a właściwie daty, specjalnych dni, są zwracane w akurat takim formacie, jaki wypadnie z maszyny losującej bazy danych (via Django). Ten output_date_format wygląda na raczej dostosowany do tego, żeby zapytanie do bazy było dobrze napisane (co z drugiej strony oczywiście oznacza zapewne dokładnie ten sam format, ale zależność przyczynowo-skutkowa jest inna, niż sugeruje sformułowanie z komentarza).
Trzeba to jakoś przeformułować, bo obecnie to sugeruje, że zmieniając output_date_format uzyskamy inne sensowne zachowanie tej funkcji, a zdaje się, że możemy ją w ten sposób tylko zepsuć.
zapisy/apps/schedule/views.py
Outdated
| Both dates has to be given in 'input_date_format'. Date is returned in 'output_date_format'. | ||
| Returns 404 if one or both paramas are missing. | ||
| """ | ||
| changed_days = ChangedDay.objects.all() |
There was a problem hiding this comment.
Zdaje się, że ta funkcja różni się od poprzedniej literalnie tylko nazwą modelu, który tu odpytujemy, prawda? Zdeduplikowałbym również i je (te funkcje), choć pozostałbym przy bieżących nazwach endpointów – funkcja będzie musiała dostać dodatkowy argument mówiący, który model wybrać, i wartość tego argumentu będzie można zadać w deklaracji path w urls.py (jest na to jakiś kwarg, czego chyba gdzieś nawet używamy w projekcie).
| "W tym dniu odbywają się zajęcia ", | ||
| (x) => changedDatesMapping[changedDates[x].weekday - 1] |
There was a problem hiding this comment.
Trochę nie podobają mi się dwa osobne argumenty osobno trzymające część stałą i część zmienną komunikatu. Wydaje mi się, że byłoby nadal czytelne (tj. nie mniej niż różne inne kawałki kodu Vue 😏), gdyby bezpośrednio tutaj w trzecim (z trzech) argumentów napisać całą funkcję zwracającą cały napis w zależności od x. Swoją drogą byłoby bardziej wysokopoziomowe, gdyby argumentem takiej funkcji nie był indeks z jakiejś tam kolekcji, tylko od razu odpowiedni element.
(Część uwag z tego komentarza to tylko opinie przy których nie będę się upierał.)
| background-color: gainsboro; | ||
| } | ||
| .change-day { | ||
| background-color: beige; |
There was a problem hiding this comment.
No dobra, poważne uwagi moment: na jaki kolor pomalować garaż na rowery? Głównie to nie podoba mi się bezpośrednie użycie nazw kolorów CSS. Zamiast gainsboro użyłbym np. Bootstrapowego gray-300, zamiast beige (który też po prostu nieco mi się nie podoba) – trochę nie wiem. warning pewnie byłby za ostry, więc może go rozcieńczyć w białym? Jestem przekonany, że kiedyś widziałem jakieś bootstrapowe rodziny kolorów czy może klas z różnymi intensywnościami danego koloru bazowego, ale teraz tego nie mogę na szybko znaleźć.
BTW w nazwie klasy też mogłoby być changed-day.
Issue: #699
Dodano oznaczenie dni wolnych w aplikacji sale.
Dodano oznaczenie dni, z zajęciami z innego dnia tygodnia, w aplikacji sale