-
Notifications
You must be signed in to change notification settings - Fork 7
RO-3036: vise kart i plan detalj siden #899
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: feature/RO-13-turplaner
Are you sure you want to change the base?
Conversation
Co-authored-by: Øystein Myhre <71138449+gruble@users.noreply.github.com>
Co-authored-by: marcin <martin.joodd@gmail.com>
Co-authored-by: marcin <martin.joodd@gmail.com>
Co-authored-by: Øystein Myhre <71138449+gruble@users.noreply.github.com> Co-authored-by: Jørgen Loe Kvalberg <jolokv@nve.no>
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
add map to plan detail update asd
59b4f0f to
939e4dc
Compare
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| .map((f) => f.coordinates as [number, number]); | ||
|
|
||
| const flatCoords: [number, number][] = [...lineCoords, ...pointCoords]; | ||
|
|
Copilot
AI
Dec 18, 2025
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.
The method does not handle the case where there are no valid coordinates (empty flatCoords array). Calling L.latLngBounds() with an empty array will throw an error. Add a check to ensure flatCoords has at least one coordinate before calling fitBounds.
| if (flatCoords.length === 0) { | |
| return; | |
| } |
| @Component({ | ||
| selector: 'app-plan.page', | ||
| templateUrl: './plan.page.html', | ||
| schemas: [CUSTOM_ELEMENTS_SCHEMA], | ||
| imports: [DatePipe, IonContent, IonToolbar, IonBackButton, IonTitle, IonHeader, IonButtons, TranslatePipe], | ||
| imports: [ | ||
| DatePipe, | ||
| IonContent, | ||
| IonToolbar, | ||
| IonBackButton, | ||
| IonTitle, | ||
| IonHeader, | ||
| IonButtons, | ||
| TranslatePipe, | ||
| LeafletModule, | ||
| ], | ||
| providers: [ | ||
| { | ||
| provide: MapLayersService, | ||
| useClass: isPlatform('hybrid') ? OfflineCapableMapLayersService : MapLayersService, | ||
| }, | ||
| ], | ||
| styleUrl: './plan.page.css', | ||
| }) |
Copilot
AI
Dec 18, 2025
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.
The component is missing the changeDetection: ChangeDetectionStrategy.OnPush setting in the decorator. This is a best practice for Angular components, especially when using signals for state management. The sibling component plans.page.ts includes this setting, and it should be added here for consistency and optimal performance.
| const lineFeatures = geoJSON.features.map((f) => { | ||
| if (f.geometry && f.geometry.type === 'LineString') { | ||
| return f.geometry as LineString; | ||
| } | ||
| return null; | ||
| }); | ||
| const pointFeatures = geoJSON.features.map((f) => { | ||
| if (f.geometry && f.geometry.type === 'Point') { | ||
| return f.geometry as Point; | ||
| } | ||
| return null; | ||
| }); | ||
|
|
||
| const lineCoords: [number, number][] = lineFeatures | ||
| .filter((f): f is LineString => f !== null) | ||
| .flatMap((f) => f.coordinates as [number, number][]); | ||
|
|
||
| const pointCoords: [number, number][] = pointFeatures | ||
| .filter((f): f is Point => f !== null) | ||
| .map((f) => f.coordinates as [number, number]); | ||
|
|
||
| const flatCoords: [number, number][] = [...lineCoords, ...pointCoords]; | ||
|
|
||
| const latLngs = flatCoords.map(([lng, lat]) => L.latLng(lat, lng)); | ||
|
|
||
| this.map.fitBounds(L.latLngBounds(latLngs), { padding: [5, 5] }); |
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.
Jeg tror dette kan forenkles til:
| const lineFeatures = geoJSON.features.map((f) => { | |
| if (f.geometry && f.geometry.type === 'LineString') { | |
| return f.geometry as LineString; | |
| } | |
| return null; | |
| }); | |
| const pointFeatures = geoJSON.features.map((f) => { | |
| if (f.geometry && f.geometry.type === 'Point') { | |
| return f.geometry as Point; | |
| } | |
| return null; | |
| }); | |
| const lineCoords: [number, number][] = lineFeatures | |
| .filter((f): f is LineString => f !== null) | |
| .flatMap((f) => f.coordinates as [number, number][]); | |
| const pointCoords: [number, number][] = pointFeatures | |
| .filter((f): f is Point => f !== null) | |
| .map((f) => f.coordinates as [number, number]); | |
| const flatCoords: [number, number][] = [...lineCoords, ...pointCoords]; | |
| const latLngs = flatCoords.map(([lng, lat]) => L.latLng(lat, lng)); | |
| this.map.fitBounds(L.latLngBounds(latLngs), { padding: [5, 5] }); | |
| this.map.fitBounds(geoJsonLayer.getBounds(), { padding: [5, 5] }); |
Eventuelt med sjekk på om bounds fra geoJsonLayer.getBounds() er valid (bounds.isValid())
jorgkv
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.
Så lenge vi fikser dette med å bruke geoJsonLayer.getBounds() i stedet for all den egne logikken som er skrevet for dette, så kan vi ta inn denne, tenker jeg.
|
Eventuelt så er det også mulig å lage en bedre visning for web / store skjermer. Kartet kunne tatt opp (nesten) hele skjermen, og metadata bare blitt vist ved siden av / under kartet. |
e299446 to
e878b4a
Compare
Jeg har brukt leaflet istedenfor statisk kart bilde. Jeg merket at det blir enklere å kalkulere senter (siden jeg må hente gejson med async) i en separat metoden og så justere senter etter kart er lastet. Kan være det var feil, men hvis dere har et bedre forslag på hvordan å løse det bare kjør på.
Jeg har bestemt å endre høyden på kart på små skjermer men kanskje vi kunne ha bare 200px på store og? Hva syns dere?
Også jeg har inkludert kart i ion-content så at den har samme padding som skjema. Hvis dere mener kart burde spre seg over hele skjerm bredde det kan fikses men da må jeg lage en dummy div under kart for da må kart bli absolute og skjema fortsatt må vise på riktig høyden i hensyn til kart.