Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nowy guzik służący resetowaniu stanu filtrów przedmiotów #1143

Merged
merged 4 commits into from
Nov 29, 2021

Conversation

JMendyk
Copy link
Contributor

@JMendyk JMendyk commented Nov 28, 2021

Do CourseFilter.vue dodany zostaje guzik, który umożliwi łatwe zresetowanie stanu filtrów.

Nietrywialną częścią implementacji była decyzja, w jaki sposób żądanie zresetowania stanu filtrów ma być przekazane z CourseFilter do komponentów odpowiadających różnym filtrom, gdyż Vue.js nie dostarcza idiomatycznego sposobu komunikacji od komponentu-rodzica do komponentów-dzieci.

Przeszukując popularne rozwiązania w internetowych dyskusjach do wyboru było:

  1. przy kliknięciu wywołanie metody na każdym komponencie (zidentyfikowanym np. przez przydzielenie każdemu komponentowi filtrującemu atrybutu ref),
  2. przygotowanie ad-hoc event-busa, przez stworzenie pustego komponentu Vue.js i przekazywanie żądania jako własne zdarzenie w tymże komponencie,
  3. przekazywanie żądania jako mutacja w Vuex.

Po rozważeniu rozwiązań doszedłem do wniosków opisanych niżej i postanowiłem wybrać rozwiązanie trzecie.

  1. Komponenty z atrybutem ref są dostępne w kodzie głównego komponentu pod this.$refs, jednakże nie istnieje możliwość pewnego namespace'owania ich (np. by komponenty filtrów mieć zagregowane pod this.$refs.filters). Trzeba by zatem filtrować this.$refs używając jakiejś dodatkowej informacji.
  2. Komunikacja rodzaju event-bus jest uznawana przez autorów jak i środowisko Vue.js za antywzorzec. Gdyby w projekcie była już używana taka konstrukcja, byłbym skory z niej skorzystać, jednak nie dopatrzyłem się jej obecności.
  3. Choć ostatecznie wybrane, rozwiązanie nie wydaje się idealne gdyż zdaje się mieszać koncept zdarzeń z konceptem stanu aplikacji. Szczególnie nie podobało mi się tworzenia pola w stanie i przełączania jego wartości (inkrementacja dla liczby, lub negacja dla wartości logicznej) aby wywołać zdarzenie zmiany stanu i jego propagację do obserwatorów.
    Szczęśliwe, mutacje nie muszą modyfikować stanu, aby informacja o ich wystąpieniu została rozpropagowana. Pusta definicja clearFilters wygląda więc podejrzanie, ale umieszczony komentarz powinien uspokoić obawy czytelnika.

Ważną odnotowania bolączką implementacji, jest brak scentralizowanego - na poziomu komponentu - ustalenia jego stanu domyślnego, która teraz odbywa się raz w polu data komponentu (przy inicjalizacji), a drugi raz przy otrzymaniu informacji o wystąpieniu mutacji filters/clearFilters. Wygląda, że jest to kolejny obszar, które nie posiada w Vue.js dobrego rozwiązania.

Pewnym pomysłem jest dodanie funkcji konstruującej domyślny stan i wykorzystywanie jej w data jak i w obserwatorze mutacji. Wadą jest jednak, aby podmiana data była uniwersalna, wykorzystanie Object.assign, który wyzwala ostrzeżenia, że data powinien być jedynie odczytywany.

Innym, modyfikacja przez rodzica atrybutu key komponentów-dzieci - powoduje to jednak ponowną inicjalizację komponentu, co wydaje się nadmiarowe.


Ten PR jest częścią implementacji trwałych filtrów (patrz #1024). Całość zmian w ramach zadania jest agregowana w PRze #1129.

@JMendyk JMendyk self-assigned this Nov 28, 2021
@JMendyk JMendyk mentioned this pull request Nov 28, 2021
7 tasks
@@ -39,9 +39,17 @@ export default Vue.extend({
// Set `on` from URL only if respective key is in search params...
if (searchParams.get(this.property) === "true") {
// and it's value is `true`.
this.$data.on = true;
this.on = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tutaj zrezygnowałeś z odwoływania się do danych komponentu za pośrednictwem $data, a w większości pozostałych miejsc używasz $data. Ale z tego co widzę, w obrębie każdego komponentu jest zachowana spójność, więc pewnie nie ma potrzeby już niczego zmieniać.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tak, akurat tutaj chyba zmiana została przez pomyłkę.

Zauważyłem, że w przykładach używana jest forma this.whatever, a nie this.$data.whatever (nie pamiętam czemu w pierwszych PRach używałem tej drugiej). Niestety, w SelectFilter.vue, gdzie chciałem zmienić użycie this.$data.selected na this.selected zdenerwowałem typechecker (bo selected ma domyślnie wartość undefined). Może jeszcze jutro przed callem do tego usiądę i jeśli uda się ładnie zmienić i uszczęśliwić typechecker to wszędzie to poprawię. W przeciwnym razie zostawię tak jak jest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, udało się.

Korzystając z okazji usunąłem też jeden (de fact trzy, bo dwa zmieniłem usuwając .$data.) komentarz utworzony przeze mnie w poprzednim PRze, bo był niepotrzebnie rozwlekły.

@JMendyk JMendyk merged commit adca349 into iiuni:feature/persistent-filters Nov 29, 2021
@JMendyk JMendyk deleted the clear-filters branch November 29, 2021 13:15
mbuszka pushed a commit that referenced this pull request Jan 3, 2022
Trwałe filtry wyszukiwania (patrz #1024)

Opis problemu, rozważone oraz wybrane rozwiązanie są opisane w https://hackmd.io/6tclhbJnQVq4d9TF2iwNMg.

Lista zadań i powiązanych PRów:
- [x] #1122 (rodzaj, opiekun przedmiotu, semestr, status propozycji) ,
- [x] #1123 (tagi oraz efekty kształcenia),
- [x] #1133 (nazwa przedmiotu),
- [x] #1132 (zalecane dla pierwszego roku),
- [x] #1143,
- [x] #1124,
- [x] #1139

Co-authored-by: Łukasz Orawiec <58004501+lukasz05@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants