Trwałe filtry wyszukiwania #1129
Conversation
* Add button to clear filters' state * Add clear filters' button in offer's CourseFilter.vue * Remove .$data. usage in Select- and TextFilter * Remove mine needlessly verbose comment
| for (let i = 0; i < semesterLinks.length; i++) { | ||
| const link: Element = semesterLinks[i]; |
There was a problem hiding this comment.
Nit: Czy pętla for-of nie byłaby tutaj odpowiedniejsza
There was a problem hiding this comment.
Przy próbie bezpośredniego iterowania po semesterLinks za pomocą for-of, Vetur zgłasza błąd:
Type 'HTMLCollectionOf' must have a '[Symbol.iterator]()' method that returns an iterator. Vetur(2488)
Należałoby więc utworzyć tablicę z tego obiektu za pomocą Array.from():
| for (let i = 0; i < semesterLinks.length; i++) { | |
| const link: Element = semesterLinks[i]; | |
| for (const link of Array.from(semesterLinks)) { |
Nie byłem pewien czy takie rozwiązanie byłoby lepsze od zastosowania zwykłej pętli for.
There was a problem hiding this comment.
Myślę, że można zostawić jak jest.
| const filterableProperties = [ | ||
| "name", | ||
| "tags", | ||
| "courseType", | ||
| "effects", | ||
| "owner", | ||
| "recommendedForFirstYear", | ||
| ]; |
There was a problem hiding this comment.
Czy dałoby się (łatwo) generować tę listę na podstawie template'u?
There was a problem hiding this comment.
Komponent CourseFilter musiałby mieć dostęp do zagnieżdżonych w swoim template komponentów. W tym celu należałoby każdemu z nich przypisać identyfikator poprzez atrybut ref, tak aby móc się do nich odwoływać za pomocą this.$refs.
Wtedy listę filterableProperties można by było wygenerować następująco:
const filterableProperties = Object.values(this.$refs).map((filter: any) => filter.property);There was a problem hiding this comment.
To chyba warto tak zrobić, w kontekście deduplikacji między widokiem w courses, prototype (i pewnie niedługo też w vote).
There was a problem hiding this comment.
Zawarłem te zmiany w PR #1156.
| // As shown by `selected` type, we expect the `allLabels` to have keys | ||
| // of type `number`. However, none(?) key-obtaining methods have signature | ||
| // that would thread this information. Thus, we annotate this here. | ||
| return keys(this.allLabels) as unknown as number[]; |
There was a problem hiding this comment.
Nit: Czy nie wystarczy tutaj return keys(this.allLabels) as number[];
There was a problem hiding this comment.
Tak, ta konstrukcja wygląda bardzo sztucznie.
Bez as unknown Vetur zgłasza ostrzeżenie celem zapobiegnięcia niezamierzonej nietrywialnej zmianie informacji o typie
Conversion of type 'string[]' to type 'number[]' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
Type 'string' is not comparable to type 'number'.
Vetur(2352)
Pisząc ten kod zbyt pobieżnie zapoznałem się z jego niuansami.
Funkcja keys zwraca listę o elementach typu string, bo klucze obiektów w JS są typu string. Pozostałe typy są konwertowane do string gdy są używane na potrzeby "bycia kluczem". Jednak przykładowo Chrome w narzędziach deweloperskich wyświetlając obiekty sugeruje, że klucze są jednak liczbami 🤷♂️.
W omawianym kodzie przyjmujemy jednak że klucze są liczbami, co jest dodatkowo wzmocnione anotacją typową KVDict dla allLabels, gdzie
export interface KVDict {
[key: number]: string;
}
Moglibyśmy więc alternatywnie napisać (ku tej opcji się skłaniam)
return keys(this.allLabels).map(Numbers);
lub skorzystać z typu Map<K, V>, w którym klucze rzeczywiście są ustalonego typu, jednak praca z tą strukturą jest istotnie inna (set/get zamiast indeksowania).
There was a problem hiding this comment.
A czy jest dla nas właściwie istotne, czy te indeksy są liczbami?
There was a problem hiding this comment.
Ma to znaczenie w kontekście funkcji intersection w IntersectionFilter, która używa operatora równości SameValueZero (który patrzy m.in. na typy).
Ostatecznie wybrałem opcję z mapowaniem kluczy na wartości liczbowe w computed property: #1155.
| this.$store.subscribe((mutation, _) => { | ||
| switch (mutation.type) { | ||
| case "filters/clearFilters": | ||
| this.allLabelKeys | ||
| .filter((key) => this.selected[key]) | ||
| .forEach(this.toggle); | ||
| break; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Rozważyłbym tutaj przełączenie wartości filtrów i pojedyncze wywołanie odświeżające query string na koniec. Aktualna implementacji generuje O(n) odświeżeń adresu, co jest zauważalne przy większej ilości wybranych labelek.
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: