-
Notifications
You must be signed in to change notification settings - Fork 7
ERSAD: Add information display and information desk inputs #1652
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: master
Are you sure you want to change the base?
ERSAD: Add information display and information desk inputs #1652
Conversation
a-limyr
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.
This is a well-structured implementation that follows established patterns. The code is clean, properly integrated across all layers and moves the application towards TypeScript. I found a type issue and some other minor things that might not be an issue.
Good job!
src/models/Facilities.ts
Outdated
| export interface SiteFacilitySet { | ||
| mobilityFacilityList: MobilityFacility[]; | ||
| passengerInformationFacilityList: PassengerInformationFacility[]; | ||
| passengerInformationEquipmentList: PassengerInformationEquipment | null; |
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.
Is this supposed to be a list also? If not then perhaps the name should be passengerInformationEquipment?
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.
Good catch! This was originally due to backend's netex model (part that is bound to external library) having it as a single value (despite name suggesting it's a list), but since in the end I made it so that tiamat's own netex model has it as a real list, I think I forgot to tune up this spot along
src/modelUtils/facilitiesHelpers.js
Outdated
| } | ||
| : { ...defaultSiteFacilitySet, ...state }; | ||
| updatedEntity.quays[id].facilities = [newSiteFacilitySet]; | ||
| } else { |
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.
Would it be an idea to have a else if here to verify that the type is stopPlace? And not just assume? At least it would be more readable.
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.
Not sure, the "entity" usually implied that it's either a quay or stopPlace (e.g. type EntityType in model), so if a method was about updating an entity, maybe there aren't other use cases coming around 🤔 But then I do remember that in the UI components there been some places where the value of "type" that can be a "parking"...
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.
But maybe for the sake of readability "else if" would make it more clear, I'll add it
| STOP_TIMETABLE = "stopTimetable", | ||
| JOURNEY_PLANNING = "journeyPlanning", | ||
| INTERACTIVE_KIOSK = "interactiveKiosk", | ||
| INFORMATION_DESK = "informationDesk", |
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.
This is the only enum in use from this set of enums. Is this intentional or have you forgotten to add support for the other enums in the UI? If it is there for future implementations it is fine.
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.
These just tagged along from the backend, could be used in future (currently only info desk is) but not yet (and probably won't be done by us)
| export enum PassengerInformationFacility { | ||
| NEXT_STOP_INDICATOR = "nextStopIndicator", | ||
| STOP_ANNOUNCEMENTS = "stopAnnouncements", | ||
| PASSENGER_INFORMATION_DISPLAY = "passengerInformationDisplay", |
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.
This is the only enum in use from this set of enums. Is this intentional or have you forgotten to add support for the other enums in the UI? If it is there for future implementations it is fine.
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.
These just tagged along from the backend, could be used in future (currently only passengerInformationDisplay is) but not yet (and probably won't be done by us)
|
@a-limyr, thank you for the review, great points! The changes been made, I'll ping once the backend is merged |
|
@a-limyr, the backend is now merged 🚀 |
Summary
In facilities tab: new information display input.
In assistance tab: new information desk input.
Both are defined on a stop place level only.
Issue
The XML behind:
Unit tests