-
Notifications
You must be signed in to change notification settings - Fork 135
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
DT-6564 Navigator journey end modal #5187
Conversation
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.
Mostly clean code and good refactoring, but handling of currentLeg concept probably requires further effort.
firstLeg: realTimeLegs?.[0], | ||
lastLeg: realTimeLegs?.[realTimeLegs.length - 1], |
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.
I am not totally against this but why to pass first and last leg as a prop? Indexing the array is not that expensive or complicated.
Note that these legs are not consistently used in NaviContainer or NavicardContainer.
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.
To centralize the deduction logic instead of having identical pieces of logic in multiple components. Thus, useRealtimeLegs
becomes responsible for correctness of data from there on out.
app/component/itinerary/navigator/navigatoroutro/NavigatorOutro.js
Outdated
Show resolved
Hide resolved
navigationLogo: 'hsl/navigator-logo.svg', | ||
thumbsUpGraphic: 'hsl/thumbs-up.svg', |
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.
We are currently using our icons insvg-sprite.hsl.svg
and svg-sprite.default.svg
We should move these icons there. Using a same key would render different images based on what instance is running.(And none if there is no corresponding icon, but may throw an error, not sure)
I highly suspect we would want images for other instances as well, even though they are yet to be designed.
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.
I'd say this is out of scope of this PR
There is a bug. where user cannot pan the map or zoom in/out. |
Inspecting this at the moment |
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.
NaviContainer indexes leg array instead of using lastLeg
Gah! Will fix. |
Proposed Changes
Adds a Navigator journey end signaling modal
journey-end.mp4
Pull Request Check List
Review