-
Notifications
You must be signed in to change notification settings - Fork 0
Unify the look and feel of search across modules #1260
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
base: main
Are you sure you want to change the base?
Conversation
06d6c67 to
cc83694
Compare
Huulivoide
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Huulivoide reviewed 23 of 23 files at r1.
Reviewable status: 19 of 23 files reviewed, 3 unresolved discussions (waiting on @Jontzii)
ui/src/components/common/search/useSearchQueryParser.ts line 82 at r1 (raw file):
DEFAULT_PRIORITIES; const transportMode =
Nyt transportMode on tyyppiä Array<ReusableComponentsVehicleModeEnum | AllOptionEnum.All Pitäskö sen kuitenkin olla Array<ReusableComponentsVehicleModeEnum | AllOptionEnum.All>?
ui/src/components/routes-and-lines/search/conditions/TransportationModeCondition.tsx line 1 at r1 (raw file):
import without from 'lodash/without';
Tää on nyt ikävästi duplikoitu koko komponentti. Onks tässä jotakin hyvin oleellista eriavaisuutta, jonka takia samaa komponenttia ei vois käyttää / tehä jotain geneeristä pohjaimplementaatiota?
ui/src/components/common/search/SearchInput.tsx line 33 at r1 (raw file):
placeholder={t('search.searchPlaceholder')} onChange={(e) => onChange(e.target.value)} onKeyDown={onKeyPress}
Jos tän korjais samalla oleen formin sisällä, niin ei tarvis manuaalista enterin hallintaa.
Jontzii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 19 of 23 files reviewed, 3 unresolved discussions (waiting on @Huulivoide)
ui/src/components/common/search/SearchInput.tsx line 33 at r1 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Jos tän korjais samalla oleen formin sisällä, niin ei tarvis manuaalista enterin hallintaa.
Korjaan
ui/src/components/common/search/useSearchQueryParser.ts line 82 at r1 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Nyt transportMode on tyyppiä
Array<ReusableComponentsVehicleModeEnum | AllOptionEnum.AllPitäskö sen kuitenkin ollaArray<ReusableComponentsVehicleModeEnum | AllOptionEnum.All>?
Ajattelin alustavasti, että ei koska ajatuksena tuntui hassulta että vaihtoehto All olisi myös arrayn sisällä, koska sitähän ei voi olla kuin yksi kerrallaan, jos saat ajatuksesta kiinni 😅
ui/src/components/routes-and-lines/search/conditions/TransportationModeCondition.tsx line 1 at r1 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Tää on nyt ikävästi duplikoitu koko komponentti. Onks tässä jotakin hyvin oleellista eriavaisuutta, jonka takia samaa komponenttia ei vois käyttää / tehä jotain geneeristä pohjaimplementaatiota?
Tuon TransportationModeButton voisi varmaan sinänsä yhdistää käyttämään samaa, ainut ero taitaa olla toi että stop registryn puolella käytettiin tuota JoreStopRegistryTransportModeType tyyppinä.
Tuossa itse exportatussa komponentissahan taas on se oleellinen ero, että se stop registry puolen käyttää formia ja tällä puolella ei käytetä, joten sitä en lähtökohtaisesti ehkä yhdistäisi.
|
Previously, Jontzii (Joonas Hiltunen) wrote…
Tai no niin, tietty sitten pitäis samalla tehdä se muutos kokonaan tohon formin puolelle 🤔 |
|
No joo, varmaan voisin kokeilla miten iso homma tuo formiin vaihtaminen olisi 🤔 |
Huulivoide
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Huulivoide reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Jontzii)
ui/src/components/common/search/useSearchQueryParser.ts line 82 at r1 (raw file):
Previously, Jontzii (Joonas Hiltunen) wrote…
Ajattelin alustavasti, että ei koska ajatuksena tuntui hassulta että vaihtoehto
Allolisi myös arrayn sisällä, koska sitähän ei voi olla kuin yksi kerrallaan, jos saat ajatuksesta kiinni 😅
Joo onhan se nuin. Varmistin vaan ettei oo ristiin mennyt tyyppejä siellä ja täällä.
ui/src/components/routes-and-lines/search/conditions/TransportationModeCondition.tsx line 1 at r1 (raw file):
Previously, Jontzii (Joonas Hiltunen) wrote…
Tuon
TransportationModeButtonvoisi varmaan sinänsä yhdistää käyttämään samaa, ainut ero taitaa olla toi että stop registryn puolella käytettiin tuotaJoreStopRegistryTransportModeTypetyyppinä.Tuossa itse exportatussa komponentissahan taas on se oleellinen ero, että se stop registry puolen käyttää formia ja tällä puolella ei käytetä, joten sitä en lähtökohtaisesti ehkä yhdistäisi.
Vaikka on eri enumi ja eri tavalla valuen ja onChange handlerin otto, niin musta tuntuu että tästä ois kuitekin 80% koodista duplikaattia. Ja vois eroavat asiat sitten vaan ottaa prpsina sisään. Ja tehdä jotenkin geneerisnä komponettinas 🤔
d975ab4 to
1ecd9b5
Compare
Jontzii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noniin formiin vaihdettu onnistuneesti, pitää vielä katsoa testit läpi
Reviewable status: 9 of 37 files reviewed, 2 unresolved discussions (waiting on @Huulivoide)
ui/src/components/common/search/SearchInput.tsx line 33 at r1 (raw file):
Previously, Jontzii (Joonas Hiltunen) wrote…
Tai no niin, tietty sitten pitäis samalla tehdä se muutos kokonaan tohon formin puolelle 🤔
Tämä on nyt poistettu kokonaan, kun vaihdoin formiin tämän
ui/src/components/routes-and-lines/search/conditions/TransportationModeCondition.tsx line 1 at r1 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Vaikka on eri enumi ja eri tavalla valuen ja onChange handlerin otto, niin musta tuntuu että tästä ois kuitekin 80% koodista duplikaattia. Ja vois eroavat asiat sitten vaan ottaa prpsina sisään. Ja tehdä jotenkin geneerisnä komponettinas 🤔
Joo, muutin nyt tuon reitit ja linjat / aikataulut puolen myös käyttämään formia ja muutin tuolta pysäkkirekisterin puolelta joitain komponentteja geneeriseksi niin sai käytettyä molemmissa paikoissa
1ecd9b5 to
bfda71d
Compare
Huulivoide
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Huulivoide reviewed 32 of 32 files at r3.
Reviewable status: 36 of 38 files reviewed, 9 unresolved discussions (waiting on @Jontzii)
ui/src/components/routes-and-lines/search/SearchContainer.tsx line 47 at r3 (raw file):
const extraFiltersId = useId(); const formRef = useRef<HTMLFormElement | null>(null);
Tämmösellle ei pitäs olla mitään tarvetta 🤔
ui/src/components/common/search/utils/mapSearchConditions.ts line 7 at r3 (raw file):
import { SearchConditions } from '../useSearchQueryParser'; const mapVehicleModeToJoreTransportMode = (
Ylätason funktio määrittely function avainsanalla niin käy heti ilmi että kyseessä on funktio.
ui/src/components/common/search/utils/mapSearchConditions.ts line 8 at r3 (raw file):
const mapVehicleModeToJoreTransportMode = ( mode: (ReusableComponentsVehicleModeEnum | AllOptionEnum)[],
Lähtökohtasesti aina kaikki data readonly muodossa sisään.
ui/src/components/common/search/utils/mapSearchConditions.ts line 35 at r3 (raw file):
}; const mapJoreTransportModeToVehicleMode = (
Samat
ui/src/components/common/search/utils/mapSearchConditions.ts line 63 at r3 (raw file):
}; export const mapFiltersToSearchConditions = (
Samat
ui/src/components/common/search/utils/mapSearchConditions.ts line 72 at r3 (raw file):
}; export const mapSearchConditionsToFilters = (
Samat
ui/src/components/common/search/ExtraFilters/TransportationModeFilter.tsx line 71 at r4 (raw file):
type TransportationModeFilterProps<FormState extends FieldValues> = { readonly fieldPath: Path<FormState>;
FieldPathByValue<FormState, ReadonlyArray<ReusableComponentsVehicleModeEnum | AllOptionEnum>>
ui/src/components/common/search/ExtraFilters/TransportationModeFilter.tsx line 95 at r4 (raw file):
const isSelected = (mode: JoreStopRegistryTransportModeType) => // TS thinks that the or can be changed for ?? due to it not getting
Ylemmän muutoksen jälkeen TS osaa arpoa oikeen tyypin value muuttujalle, muutenhan se ei voi tietää että mihinkä / minkä tyyppiseen kenttään fieldPath viittaa, ja joutuu ottaan kaikki FormState:n invariantit huomioon.
ui/src/components/common/search/useSearchQueryParser.ts line 15 at r3 (raw file):
export type SearchConditions = { query: string;
Tänne vois ripotella Readonly määreet paikoilleen samalla ku tätä nyt kopeloidaan.
bfda71d to
5ef0dca
Compare
Jontzii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 36 of 38 files reviewed, 9 unresolved discussions (waiting on @Huulivoide)
ui/src/components/common/search/useSearchQueryParser.ts line 15 at r3 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Tänne vois ripotella Readonly määreet paikoilleen samalla ku tätä nyt kopeloidaan.
Done.
ui/src/components/common/search/utils/mapSearchConditions.ts line 7 at r3 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Ylätason funktio määrittely
functionavainsanalla niin käy heti ilmi että kyseessä on funktio.
Done.
ui/src/components/common/search/utils/mapSearchConditions.ts line 8 at r3 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Lähtökohtasesti aina kaikki data readonly muodossa sisään.
Done.
ui/src/components/common/search/utils/mapSearchConditions.ts line 35 at r3 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Samat
Done.
ui/src/components/common/search/utils/mapSearchConditions.ts line 63 at r3 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Samat
Done.
ui/src/components/common/search/utils/mapSearchConditions.ts line 72 at r3 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Samat
Done.
ui/src/components/routes-and-lines/search/SearchContainer.tsx line 47 at r3 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Tämmösellle ei pitäs olla mitään tarvetta 🤔
Done.
ui/src/components/common/search/ExtraFilters/TransportationModeFilter.tsx line 71 at r4 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
FieldPathByValue<FormState, ReadonlyArray<ReusableComponentsVehicleModeEnum | AllOptionEnum>>
Done.
ui/src/components/common/search/ExtraFilters/TransportationModeFilter.tsx line 95 at r4 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Ylemmän muutoksen jälkeen TS osaa arpoa oikeen tyypin value muuttujalle, muutenhan se ei voi tietää että mihinkä / minkä tyyppiseen kenttään fieldPath viittaa, ja joutuu ottaan kaikki FormState:n invariantit huomioon.
En tiedä teinkö jotain väärin, mutta en saanut ihan täysin toimimaan niinkuin pitäisi. Laitoin tohon alle tollasen typedValue, joka nyt ainakin vähän auttoi.
Huulivoide
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Huulivoide reviewed 7 of 8 files at r5, all commit messages.
Reviewable status: 37 of 38 files reviewed, 3 unresolved discussions (waiting on @Jontzii)
ui/src/components/common/search/ExtraFilters/TransportationModeFilter.tsx line 92 at r5 (raw file):
const { field: { value, onBlur, onChange }, } = useController<FormState, typeof fieldPath>({
Jos tästä poistaa noi manuaaliset tyyppi annotit useController ja antaa sen automaagisesti päätellä ne name parametrin perusteella, niin sit pitäs toimii ilman että tarvii erikseen kastia.
Ainakin: ui/src/components/forms/common/DateInputField.tsx toimii silleen.
This change is