feat: add support for rendering agency name with the stop name in map popup #857
Conversation
| /** | ||
| * A method to request the agency for a given stop ID. If this method is not passed, the agency | ||
| * for a given stop ID will not be displayed in the popup. | ||
| */ | ||
| requestAgencyForStop?: (stopId: string) => void |
There was a problem hiding this comment.
Why do we have this as well as the map. Would it be possible to just have this instead? Feels cleaner
There was a problem hiding this comment.
Not possible. In OTP-RR we are requesting this data from OTP and it gets stored in Redux, so we wouldn't be able to turn this into an async function that resolves when the request completes. (You could maybe do some hacky thing with a Promise + useEffect that resolves the promise when the redux state updates but... no)
Passing the stop/agency object in allows us to reactively update this component when the data is available.
There was a problem hiding this comment.
Can we get rid of this line then? The method is never defined
There was a problem hiding this comment.
It's defined in the OTP-RR PR. https://github.com/opentripplanner/otp-react-redux/pull/1433/files
miles-grant-ibigroup
assigned
daniel-heppner-ibigroup and unassigned
miles-grant-ibigroup
daniel-heppner-ibigroup
assigned
miles-grant-ibigroup and unassigned
daniel-heppner-ibigroup
miles-grant-ibigroup
assigned
daniel-heppner-ibigroup and unassigned
miles-grant-ibigroup
daniel-heppner-ibigroup
assigned
binh-dam-ibigroup and
alec-georgoff and unassigned
daniel-heppner-ibigroup
|
Adding @alec-georgoff so he can take a look at how this PR accomplishes its goals. It's a good lesson in how confusing the data flow can be in OTP-RR... |
packages/map-popup/src/util.ts
Outdated
| entity.id in stopIdAgencyMap && | ||
| "code" in entity | ||
| ) { | ||
| stationName = `${stationName} (${stopIdAgencyMap[entity.id].name} ${ |
There was a problem hiding this comment.
Do we need to pass an entire map if the only important thing is the agency name of the stop? Can we just pass an agency id to the popup?
daniel-heppner-ibigroup
assigned
miles-grant-ibigroup and
binh-dam-ibigroup and unassigned
daniel-heppner-ibigroup and
miles-grant-ibigroup
|
Based on discussions a week ago about retrieving the agency name, it turns out what we actually want is the feed name. Turned out to be a major simplification! |
| W Burnside & SW 2nd (TriMet 9526) | ||
| </header> | ||
| <p class="styled__PopupRow-sc-12kjso7-2 ckOOWr"> | ||
| <strong> | ||
| Stop ID: 9526 |
There was a problem hiding this comment.
This story repeats the stop code in the title and the stop name, is that intended? In that case, should "TriMet 9526" only appear in Stop Code?
There was a problem hiding this comment.
I think that's correct. The stop ID shows the stop code, which is correct. And the stop code exists in the title to match the geocoder output, so that's also correct. But maybe I don't understand your comment.
daniel-heppner-ibigroup
assigned
miles-grant-ibigroup and unassigned
daniel-heppner-ibigroup
miles-grant-ibigroup
assigned
daniel-heppner-ibigroup and unassigned
miles-grant-ibigroup and
alec-georgoff
daniel-heppner-ibigroup
merged commit e75ac85
into
opentripplanner:master
This PR adds support for rendering the agency name, stop code, and stop name in the map popup used in otp2 tile overlay. It does this by accepting a prop containing a mapping of stop IDs to agencies, since this information is not available in the vector tile data. It also provides a callback prop so the component can tell its parent when a stop has been clicked on and the data is needed.
Paired with opentripplanner/otp-react-redux#1433