Skip to content

give map popup the agency names so it can render them in the stop popup #1433

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Sep 3, 2025
Merged

give map popup the agency names so it can render them in the stop popup #1433

merged 8 commits into from
Sep 3, 2025

Conversation

daniel-heppner-ibigroup
Copy link
Contributor

Description:
We needed to render the agency name for a stop in the stop popup. Sadly this information is not easy to get, it requires another network request. This change provides a callback for the tile overlay to request information about a stop ID, and then gives it a map of all the stops from the transit index along with their agency object. Note that because the stops can have multiple agencies, we just take the first one. Feel free to suggest a different behavior for shared stops.

@daniel-heppner-ibigroup daniel-heppner-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Jun 25, 2025
@daniel-heppner-ibigroup
Copy link
Contributor Author

Blocked by opentripplanner/otp-ui#857

@daniel-heppner-ibigroup daniel-heppner-ibigroup mentioned this pull request Jun 26, 2025
Merged
@daniel-heppner-ibigroup
Copy link
Contributor Author

This is blocked as in we can't merge it until we have the new version numbers, but it can be reviewed!

@miles-grant-ibigroup
Copy link
Collaborator

This is blocked as in we can't merge it until we have the new version numbers, but it can be reviewed!

otp-ui is ready for merge! I'll wait until this PR has the updated package version and merge conflicts fixed

@daniel-heppner-ibigroup daniel-heppner-ibigroup removed the BLOCKED Blocked (waiting on another PR to be merged) label Jul 29, 2025
@miles-grant-ibigroup
Copy link
Collaborator

Is this ready for review? If so, can you update package versions and fix merge conflicts?

@daniel-heppner-ibigroup
Copy link
Contributor Author

done!

miles-grant-ibigroup
miles-grant-ibigroup requested changes Aug 6, 2025
View reviewed changes
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions

lib/components/map/default-map.tsx Outdated
Comment on lines 178 to 184
getAgencyFromStopId = async (stopId: string) => {
this.props.findStopTimesForStop({
date: getCurrentDate(),
stopId
})
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! no longer needed

lib/components/map/default-map.tsx
@@ -462,6 +478,7 @@ class DefaultMap extends Component {
const mapStateToProps = (state) => {
const activeSearch = getActiveSearch(state)
const viewedRoute = state.otp?.ui?.viewedRoute?.routeId
const stops = state.otp.transitIndex.stops
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if transitIndex is undefined

lib/components/map/default-map.tsx
@@ -486,6 +503,7 @@ const mapStateToProps = (state) => {
bikeRentalStations: state.otp.overlay.bikeRental.stations,
carRentalStations: state.otp.overlay.carRental.stations,
config: state.otp.config,
feeds: state.otp.transitIndex.feeds,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if transit index is undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it won't be, it's defined in the initial state in the reducer.

miles-grant-ibigroup
miles-grant-ibigroup requested changes Aug 8, 2025
View reviewed changes
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is making the stop code appear twice. Is there any way to make this optional? Screenshot 2025-08-08 at 3 46 24 PM

miles-grant-ibigroup
miles-grant-ibigroup approved these changes Aug 28, 2025
View reviewed changes
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well and code looks good! STill seeing some funky behavior when clickign on stations but I guess that's ok?

@binh-dam-ibigroup
Copy link
Collaborator

I am not seeing the updated text using other languages (e.g. Spanish). Any ideas? I'll try a to find the cause of that.

binh-dam-ibigroup
binh-dam-ibigroup approved these changes Sep 2, 2025
View reviewed changes
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Necessary changes for the scope of this PR seem fine. Out of scope for this PR: Better i18n handling for stop id formatting in other languages.

@daniel-heppner-ibigroup daniel-heppner-ibigroup merged commit 405d1cd into dev Sep 3, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binh-dam-ibigroup binh-dam-ibigroup binh-dam-ibigroup approved these changes

@miles-grant-ibigroup miles-grant-ibigroup miles-grant-ibigroup approved these changes

Assignees

@daniel-heppner-ibigroup daniel-heppner-ibigroup

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daniel-heppner-ibigroup @miles-grant-ibigroup @binh-dam-ibigroup @danielhep