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

Allow point annotations to scale with zoom. #628

Merged
merged 3 commits into from
Oct 20, 2016
Merged

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Oct 19, 2016

Added a scaled property. If false, points are a fixed size. If true, the first time the point annotation is rendered, it is changed to the current zoom level. If a zoom level, the radius of the point is scaled with the map so that it is the specified radius value at the scaled property's zoom level.

Added a scaled property.  If false, points are a fixed size.  If true, the first time the point annotation is rendered, it is changed to the current zoom level.  If a zoom level, the radius of the point is scaled with the map so that it is the specified radius value at the scaled property's zoom level.
@manthey
Copy link
Contributor Author

manthey commented Oct 19, 2016

This addresses issue #624. ping @kotfic

@codecov-io
Copy link

codecov-io commented Oct 19, 2016

Current coverage is 85.52% (diff: 100%)

Merging #628 into master will increase coverage by 0.07%

@@             master       #628   diff @@
==========================================
  Files            85         85          
  Lines          8237       8264    +27   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7039       7068    +29   
+ Misses         1198       1196     -2   
  Partials          0          0          

Powered by Codecov. Last update 33cb7a4...72e3135

@aashish24
Copy link
Member

@manthey wouldn't it be better if we have this option on the pointFeature (and just pass a flag to the shader). There might be some other complications though which I haven't thought about it but may be you did?

@manthey
Copy link
Contributor Author

manthey commented Oct 19, 2016

@aashish24 We could, but I'd rather leave that as a separate task to do later (and if we do, this could be refactored). Fundamentally, this works in all renderers, but adds a bit of overhead in that it needs to perform some action on zoom. If we add it to the feature itself, it would be best to go into each renderer and support changing the point size in a more direct way (so we don't have to hook to the zoom event).

As before, the user can make radius a function, e.g. function (d) { return 10 * Math.pow(2, map.zoom() - 6); }, and then set feature.modified() in the zoom event callback.

@aashish24
Copy link
Member

@aashish24 We could, but I'd rather leave that as a separate task to do later (and if we do, this could be refactored). Fundamentally, this works in all renderers, but adds a bit of overhead in that it needs to perform some action on zoom. If we add it to the feature itself, it would be best to go into each renderer and support changing the point size in a more direct way (so we don't have to hook to the zoom event).

As before, the user can make radius a function, e.g. function (d) { return 10 * Math.pow(2, map.zoom() - 6); }, and then set feature.modified() in the zoom event callback.

Thanks after our discussion yesterday this is +1 LGTM from me. 👍

@manthey manthey merged commit 27b090e into master Oct 20, 2016
@manthey manthey deleted the scaled-point-annotations branch October 20, 2016 13:28
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