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

Expose getImage to public API #5775

Merged
merged 4 commits into from
Dec 1, 2017
Merged

Expose getImage to public API #5775

merged 4 commits into from
Dec 1, 2017

Conversation

alex3165
Copy link
Contributor

@alex3165 alex3165 commented Nov 30, 2017

This PR expose getImage method from image manager to the public API. It should address #5735.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

thanks for the contribution! I noticed one missing return statement but I also think it would be a good idea to guard/return an error when no id is passed or when the id is not a string.

src/ui/map.js Outdated
* @param id The ID of the image.
*/
getImage(id: string) {
this.style.getImage(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

you need a return statement here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad 👍

@jfirebaugh
Copy link
Contributor

What's the return type of getImage? Is that something we're comfortable making part of the public API?

@alex3165
Copy link
Contributor Author

Thanks, I addressed all the comments and squashed my commits

@mollymerp
Copy link
Contributor

Good point @jfirebaugh@alex3165 would returning a boolean and renaming the method hasImage suffice for your use-case? I'm not sure how useful it would be to return what is stored in the image manager: StyleImage

getImage(id: string): ?StyleImage {
return this.imageManager.getImage(id);
}

removeImage(id: string) {
if (!this.imageManager.getImage(id)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use the new Style#getImage method here.

src/ui/map.js Outdated
*
* @param id The ID of the image.
*/
getImage(id: string): ?StyleImage {
Copy link
Contributor

Choose a reason for hiding this comment

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

the build is failing because the type StyleImage was not imported so flow is erroring. I think we should rename the method hasImage and change the return type to boolean

@alex3165
Copy link
Contributor Author

alex3165 commented Dec 1, 2017

Yes hasImage is good enough! 👍

@mollymerp
Copy link
Contributor

Thanks @alex3165 !

@mollymerp mollymerp merged commit 04b3c35 into mapbox:master Dec 1, 2017
@alex3165
Copy link
Contributor Author

alex3165 commented Dec 4, 2017

Hey, @mollymerp when do you plan to release this?

@mollymerp
Copy link
Contributor

This will go out with the v0.43.0 release – i'd estimate sometime in the next 3-5 weeks

@areichman
Copy link

@mollymerp Is there an equivalent method that could be exposed for controls? If so, I can create a new ticket.

The use case is switching map styles. Since there is no "change base layer" ability like in Leaflet, we store references to all active layers in our app state and then re-add those sources/layers in GL. It appears that controls are not actually removed during this process so when we re-add them we get duplicate copies of the control.

@jinkwon
Copy link

jinkwon commented Dec 11, 2017

Great!!

@mollymerp
Copy link
Contributor

@areichman – no we don't expose a method for Controls. For DOM elements added to the map (see #5821 (comment)), we generally don't keep copies in the Map state to minimize memory use and API sprawl. Like Markers, Controls are not part of the Map's stylesheet, so changing styles won't remove the controls you've added unless you create a new mapboxgl.Map(... instance. If you do need to remove the controls when you switch styles, I suggest you save a copy of the Control instance and then invoke Map#removeControl as needed.

@areichman
Copy link

@mollymerp That's what I ended up doing. I am storing the control reference for other reasons and can just check for the presence of control._map to know if it has already been added. Thanks!

@alpha-beta-soup
Copy link

alpha-beta-soup commented Nov 10, 2018

I'm not sure what this internal getImage method returns, but just putting it out there that it would be great to use something like that to obtain the symbol used by a Mapbox style, so that a legend can be automatically populated within an application that depends on Mapbox GL JS.

@ryanhamley
Copy link
Contributor

ryanhamley commented Nov 12, 2018

I'm not sure what your use case is @alpha-beta-soup but there is a listImages method that returns the names of all images in the style's sprite sheet.

@alpha-beta-soup
Copy link

That's a handy method actually. My use case is to create automated legends from a layer. I do this with circle, symbol and polygon layers at the moment using my own (brittle) method that works because I have tightly-defined, known inputs (and graceful failure). But point symbol layers only work with a reference to the name of the symbol in the sprite sheet—and then I use that name to select the correct SVG to include in the legend. That means I need to bundle the sprites/symbols into both my application and into the spritesheet.

I don't know if what I want to do is at all possible, but I just think it would be useful to get the actual image used in the spritesheet (or some kind of remote reference) to include in a legend, without having that separation between the symbols in Mapbox and those in the application. Then someone can add symbols to the spritesheet via Mapbox Studio without rebuilding the application.

I'll include a gif. The legend for the area layer is determined dynamically, entirely based on the content of the style loaded when the application starts. Ideally I'd achieve the same for symbol layers too. In this example, all layers are pre-included into the style, and the UI simply toggles layer visibility.

peek 2018-11-15 10-36

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.

7 participants