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

Changing MBVisualInstructionComponentType.image -> .icon #275

Closed
wants to merge 2 commits into from

Conversation

JThramer
Copy link
Contributor

Updating MBVisualInstructionComponentType to reflect new changes on API. This is to reflect that the icon component may have only text, and thus should be rendered as a generic route shield. Supports mapbox/mapbox-navigation-ios#1190.

/cc @mapbox/navigation-ios

@bsudekum
Copy link

bsudekum commented May 10, 2018

@JThramer this looks like just the enum name is changing here, is that what you are going for?

*/
case image
case icon
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 sure this change is strictly necessary. “Image” and “icon” are roughly synonymous in this context. We went with image for consistency with the VisualInstructionComponent.imageURL property, which is still present regardless of the API change.

@JThramer
Copy link
Contributor Author

@bsudekum @1ec5 The idea here in my mind was that it was kinda silly to have a component type with the name of image if it may not actually contain an image.

@1ec5
Copy link
Contributor

1ec5 commented May 11, 2018

Hm, I guess I would make the same point about icon. I think type: icon in the API response should be seen as an indication that the component would be displayed as an image, whether or not that image comes from the API.

For context, this component type was originally conceived as a ref type for all road codes, but there was a consensus to move away from semantic types in favor of presentational types. Perhaps a future refactor could use subclasses instead of this enumeration to more clearly distinguish between the various kinds of components.

@JThramer
Copy link
Contributor Author

Consensus was gained that this change is superfluous. Closing.

@JThramer JThramer closed this May 15, 2018
@1ec5 1ec5 added backwards incompatible changes that break backwards compatibility of public API and removed backwards incompatible changes that break backwards compatibility of public API labels Dec 18, 2018
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.

None yet

3 participants