Skip to content
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

Add Mapillary links in itinerary #521

Merged
merged 8 commits into from
Feb 23, 2022

Conversation

miles-grant-ibigroup
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup commented Jan 12, 2022

Depends on opentripplanner/otp-ui#328

Adds Mapillary key to config to make use of itinerary-body updates. Also uses the callback to render the Mapillary iframe in a popover in the map, rather than as a popup.
Screen Shot 2022-01-12 at 4 23 59 PM

There is a lot of auto formatter noise in this PR, apologies. The actual changes are minimal, and mostly clone the elevation popup reducers to store a Mapillary ID in redux state that is then read by a new component which renders the iframe (and could theoretically render something else)

To enable the new feature, the active config must have a maxillary -> key set (see example config). You can register for a free key at https://www.mapillary.com/dashboard/developers

@miles-grant-ibigroup miles-grant-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Jan 12, 2022
@miles-grant-ibigroup miles-grant-ibigroup self-assigned this Jan 12, 2022
@miles-grant-ibigroup miles-grant-ibigroup removed the BLOCKED Blocked (waiting on another PR to be merged) label Feb 8, 2022

# API key to make Mapillary API calls. These are used to show street imagery.
# mapillary:
# key: <Mapillary Key>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing an API key provided from Mapillary on the dev portal. They provide a client token and a client secret, at least with the way I registered the app. What do I put in this field? Can we call this something else? Or clarify in the comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made a clearer comment in f68f925!

@miles-grant-ibigroup miles-grant-ibigroup removed their assignment Feb 22, 2022
Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

I think my only problem with this is that it's not immediately clear what I'm looking at which I click on a Mapillary link.

For example, if I click on the icon for a "Turn left onto X road", am I looking at the imagery for X road or the road I was on right before the turn? I think the current language sounds like I am going to be looking at the intersection where I need to turn, but it seems like it is showing the image from right after the turn.

Is there any way we could clarify this? Maybe that would be better put in the itinerary-body in OTP UI. Or maybe it's not that important.

@miles-grant-ibigroup
Copy link
Collaborator Author

Yeah I agree with you @daniel-heppner-ibigroup. Definitely a shortcoming. The original design called for highlighting the streets at a certain zoom level and allowing a click to show you the image at that location. Sadly, this doesn't seem to work very well without vector tiles. Check out opentripplanner/otp-ui#327 for more

Thank you for a thoughtful review though!

@miles-grant-ibigroup miles-grant-ibigroup merged commit 61b4498 into dev Feb 23, 2022
@miles-grant-ibigroup miles-grant-ibigroup deleted the mapillary-links-in-itinerary branch February 23, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants