Create tram route#1448
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
PasiVuohijoki
left a comment
There was a problem hiding this comment.
Rebasa myös kaipaileepi.
@PasiVuohijoki reviewed 20 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on patricvincit).
ui/src/components/map/routes/hooks/useRouteInfo.ts line 104 at r1 (raw file):
? mapRouteFormToInput(editedRouteData.metaData) : undefined, routeVehicleMode: editedRouteData.lineInfo?.primary_vehicle_mode,
Tässä vois olla fallbackina tuo reduxista löytyvä editedRouteData.vehicleMode niin ei RouteStopsOverlay pamauta nullia kun luodaan uutta reittiä.
Eli niinku näi:
routeVehicleMode: editedRouteData.lineInfo?.primary_vehicle_mode ?? editedRouteData.vehicleMode,
e741d9e to
0241609
Compare
patricvincit
left a comment
There was a problem hiding this comment.
Jep, ja korjailin myös nyt testit tolta "lisää reitti" painikkeelta kun siihen tuli noi useemmat valinnat. Ja lisäsin testin ratikkareitin luomiselle siihen asti kun sitä nyt tässä vaiheessa voi testata
@patricvincit made 2 comments.
Reviewable status: 16 of 23 files reviewed, 1 unresolved discussion (waiting on PasiVuohijoki).
ui/src/components/map/routes/hooks/useRouteInfo.ts line 104 at r1 (raw file):
Previously, PasiVuohijoki (Pasi Vuohijoki) wrote…
Tässä vois olla fallbackina tuo reduxista löytyvä editedRouteData.vehicleMode niin ei RouteStopsOverlay pamauta nullia kun luodaan uutta reittiä.
Eli niinku näi:
routeVehicleMode: editedRouteData.lineInfo?.primary_vehicle_mode ?? editedRouteData.vehicleMode,
Done.
Huulivoide
left a comment
There was a problem hiding this comment.
@Huulivoide reviewed 22 files and all commit messages, and made 3 comments.
Reviewable status: 22 of 23 files reviewed, 4 unresolved discussions (waiting on PasiVuohijoki and patricvincit).
ui/src/api/routing.ts line 70 at r2 (raw file):
} export const getBusRoute = async (
Ylätason funktio function määrittelynä
ui/src/components/forms/route/ChooseLineDropdown.tsx line 48 at r2 (raw file):
query, value, undefined,
Tää pistää silmään että observationDate on undefined. Missä muualla tätä useChooseLineDropdown hookkia käytetään? Onko siellä observationDate speksattu?
ui/src/components/map/routes/RouteStopsOverlay.tsx line 40 at r2 (raw file):
}; const getVehicleModeIcon: Readonly<
get viittais functioon, joku vehicleModeIconMap vois olla kuvaavampi nimi. Toisaalta tätä samaa taitaa nyt olla useemmassa eripaikassa periaatteessa kopipastana. Vois kattoo joskoi sais jonku geneersien apu palikan jonnekin kivaan kohtaan.
0241609 to
f7ba46d
Compare
patricvincit
left a comment
There was a problem hiding this comment.
@patricvincit made 3 comments.
Reviewable status: 14 of 23 files reviewed, 4 unresolved discussions (waiting on Huulivoide and PasiVuohijoki).
ui/src/api/routing.ts line 70 at r2 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Ylätason funktio
functionmäärittelynä
Done.
ui/src/components/forms/route/ChooseLineDropdown.tsx line 48 at r2 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Tää pistää silmään että observationDate on undefined. Missä muualla tätä useChooseLineDropdown hookkia käytetään? Onko siellä observationDate speksattu?
Toi on ainoastaan käytössä tässä ChooseLineDropdown komponentissa tällä hetkellä. Varmistin vielä miten tämän haluttaisiin käytännössä toimivan: nykyinen toiminnallisuus on juurikin se mitä halutaan. Eli reittiä luodessa modaalilla observationDate olis aina tämä nykyinen päivä, riippumatta siitä mitä kartalla on valittu filttereiksi. Tuolla hookissa on nyt tällä hetkellä fallbackina today.toISO(), jos tota observationDatea ei anneta eli tämän kautta sitten toiminnallisuus on oikea. Olisko tässä parempi antaa sitten kuitenkin undefinedin sijaan joku arvo? Aiemmin se oli myös undefined, mutta nyt se tuli vasta selkeemmin näkyviin koska tuli myös neljäs argumentti joka pakotti ton laittamisen.
ui/src/components/map/routes/RouteStopsOverlay.tsx line 40 at r2 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
get viittais functioon, joku vehicleModeIconMap vois olla kuvaavampi nimi. Toisaalta tätä samaa taitaa nyt olla useemmassa eripaikassa periaatteessa kopipastana. Vois kattoo joskoi sais jonku geneersien apu palikan jonnekin kivaan kohtaan.
Tein tolle yhen mapin tän tiedoston sisällä. Mutta tulikin vielä vastaan, että vastaavanlaista mappailua onkin ympäri projektia. Meinasitko tässä just tän tiedoston sisällä olevien laittamista yhtenäisemmäks, vai ihan yleisesti näitä muitakin paikkoja mistä tota vastaavaa löytyy
Huulivoide
left a comment
There was a problem hiding this comment.
@Huulivoide reviewed 10 files, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on PasiVuohijoki and patricvincit).
ui/src/components/forms/route/ChooseLineDropdown.tsx line 48 at r2 (raw file):
Previously, patricvincit (Patric Kangasmäki) wrote…
Toi on ainoastaan käytössä tässä ChooseLineDropdown komponentissa tällä hetkellä. Varmistin vielä miten tämän haluttaisiin käytännössä toimivan: nykyinen toiminnallisuus on juurikin se mitä halutaan. Eli reittiä luodessa modaalilla observationDate olis aina tämä nykyinen päivä, riippumatta siitä mitä kartalla on valittu filttereiksi. Tuolla hookissa on nyt tällä hetkellä fallbackina today.toISO(), jos tota observationDatea ei anneta eli tämän kautta sitten toiminnallisuus on oikea. Olisko tässä parempi antaa sitten kuitenkin undefinedin sijaan joku arvo? Aiemmin se oli myös undefined, mutta nyt se tuli vasta selkeemmin näkyviin koska tuli myös neljäs argumentti joka pakotti ton laittamisen.
Eli tätä koko parametria ei koskaan käytetä mihinkään? Sen vois kokonaan pudottaa pois
ui/src/components/map/routes/RouteStopsOverlay.tsx line 40 at r2 (raw file):
Previously, patricvincit (Patric Kangasmäki) wrote…
Tein tolle yhen mapin tän tiedoston sisällä. Mutta tulikin vielä vastaan, että vastaavanlaista mappailua onkin ympäri projektia. Meinasitko tässä just tän tiedoston sisällä olevien laittamista yhtenäisemmäks, vai ihan yleisesti näitä muitakin paikkoja mistä tota vastaavaa löytyy
Sitä meinasin, että jos nää on identtiset kopiot monessa paikassa, niin vois pitää keskitetysti yhdessä paikassa yhtä listausta vaan.
471b700 to
5f6673d
Compare
patricvincit
left a comment
There was a problem hiding this comment.
@patricvincit made 1 comment.
Reviewable status: 20 of 26 files reviewed, 3 unresolved discussions (waiting on Huulivoide and PasiVuohijoki).
ui/src/components/map/routes/RouteStopsOverlay.tsx line 40 at r2 (raw file):
Previously, Huulivoide (Jesse Jaara) wrote…
Sitä meinasin, että jos nää on identtiset kopiot monessa paikassa, niin vois pitää keskitetysti yhdessä paikassa yhtä listausta vaan.
Vähän nyt vielä muutin tätä rakennetta: Näytti tuolle taustavärin hakemiselle löytyvän vastaava jo colors utilsseista nimellä mapVehicleModeToRouteColor. Sitten LineDetailsByIdPage komponentissa oli vastaava ikonin luonti käytännössä täysin samana (vähän eri toteutus tosin), niin tein uuden utils tiedoston (liekö icons.ts hyvä nimi..?), josta molemmat paikat nyt käyttää tota ikonin mappausta. Ja mitä ilmeisimmin sitä ollaan myös tulevaisuudessa muuallakin käyttämässä, niin geneerinen utils tiedosto on hyvä kyllä tohon
Huulivoide
left a comment
There was a problem hiding this comment.
@Huulivoide reviewed 7 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on PasiVuohijoki).
ca123bd to
e41ecab
Compare
PasiVuohijoki
left a comment
There was a problem hiding this comment.
@PasiVuohijoki reviewed 5 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on patricvincit).
Previously drawing a route would always create a bus route. Now the button displays selection for tram or bus route and adjusts the line search results based on the users selection
Late update on snapping line could make it visible after route creation as unmounting the draw layer does not currently clean up the debounced calls.
Change from hardcoded bus icon to actual vehicle type. Also add a new icons helper function and use it in the LineDetailsByIdPage
Changing to direct mode while snapping line is undefined results in a crash.
After cancelling drawing mode, the crosshair cursor persisted. Fix by adding a clean up for the cursor
Previously it would open the route creation modal but that functionality is not wanted now.
Now the user can create both bus and tram routes from the same view so tests needed to be updated
ChooseLineDropdown should always default to today in the lines results.
showIconForVehicleMode component got replaced by vehicleModeIconMapping utils
e41ecab to
e75d4c0
Compare
This change is