AB#168 - Improve indoor routing level info #5595
Conversation
…ve-indoor-routing-level-info
vesameskanen
left a comment
vesameskanen
left a comment
There was a problem hiding this comment.
Indoor step rendering is fragmented especially in NaviCardExtension and quite hard to follow. Would it be possible to move most required rendering code and logic to IndoorContainer class? So, move leg rendering would be something like:
if (showIndoorRoute) {
<IndoorContainer ...>
{ Steps.length === 1 && stopInformation() }
}
| import { isKeyboardSelectionEvent } from '../../util/browser'; | ||
| import Icon from '../Icon'; | ||
|
|
||
| export default function IndoorRouteInfo( |
There was a problem hiding this comment.
In UI context, Route refers to transit lines such as tram 7 or bus 500. Users search itineraries, not routes. In finnish we have to share the word Reitti for many purposes but in code we can avoid that.
I have a feeling that dropping Route from the names would be good:
- IndoorInfo
- IndoorStep
- IndoorSteps or IndoorLeg
- navigator/IndoorButton
etc.
| const [defaultBackgroundImageUrl, setDefaultBackgroundImageUrl] = useState(); | ||
| const [indoorBackgroundImageUrl, setIndoorBackgroundImageUrl] = useState(); | ||
| useEffect(() => { | ||
| Promise.all([ | ||
| import( | ||
| /* webpackChunkName: "dotted-line" */ `../../configurations/images/default/dotted-line.svg` | ||
| ), | ||
| import( | ||
| /* webpackChunkName: "indoor-dotted-line" */ `../../configurations/images/default/indoor-dotted-line.svg` | ||
| ), | ||
| ]).then(([defaultImageUrl, insideImageUrl]) => { | ||
| setDefaultBackgroundImageUrl(`url(${defaultImageUrl.default})`); | ||
| setIndoorBackgroundImageUrl(`url(${insideImageUrl.default})`); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
This convention you copied from old code looks terrible. It really makes no sense to lazy load tiny svg snippets one by one, and the code is far too verbose and complicated. I believe it is possible to convert svg entry dynamically to css compatible form, or at least use one of the methods here:
https://www.svgbackgrounds.com/how-to-add-svgs-with-css-background-image/
Url approach still fetches images one by one but at least looks simple. Embedding the data as part of css is probably the best way.
| import { | ||
| subwayTransferUsesSameStation, | ||
| getIndoorRouteLegType, | ||
| getIndoorStepsWithVerticalTransportationUse, |
There was a problem hiding this comment.
Somehow postfix 'Use' at the end does not help but makes things harder to understand.
| useEffect(() => { | ||
| // If there is only one indoor routing step, always show it. | ||
| if (indoorRouteSteps.length === 1) { | ||
| setShowIntermediateSteps(true); | ||
| } | ||
| }, [indoorRouteSteps]); |
There was a problem hiding this comment.
Do not use this kind of effect. getIndoorStepsWithVerticalTransportationUse regenerates new array in each call and effect triggers at every render. I suggest initializing showIntermediateSteps state directly with one single call to getIndoorStepsWithVerticalTransportationUse.
| ) { | ||
| return getIndoorSteps(previousLeg, leg, nextLeg).filter(step => | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| isVerticalTransportationUse(step?.feature?.__typename), |
There was a problem hiding this comment.
Is it really possible that step list contains null entries? The same applies to step.feature - can it be null and what happens when rendering code gets such steps?
If so I suggest filtering invalid steps here so that the rest of the code can trust them.
| } | ||
|
|
||
| export function getEntranceWheelchairAccessibility(leg) { | ||
| return leg?.steps?.find( |
There was a problem hiding this comment.
I'd expect leg is not null. Let the code crash if somebody does not know how to use this func.
| export function getEntranceObject(previousLeg, leg) { | ||
| const entranceObjects = leg?.steps |
There was a problem hiding this comment.
leg is not null, no need for ?
| export function createFeatureObjects(objects) { | ||
| return objects.map(object => ({ | ||
| type: 'Feature', | ||
| properties: { ...object.properties }, |
There was a problem hiding this comment.
Why do you copy the object?
Proposed Changes