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

fix icon-size for small data-driven values #7125

Merged
merged 2 commits into from
Aug 14, 2018
Merged

fix icon-size for small data-driven values #7125

merged 2 commits into from
Aug 14, 2018

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Aug 14, 2018

fix #7017, fix #7066

Fixes both issues by scaling icon-size and text-size by 256 instead of 10. It can now represent values from 0 - 256 with precision of 1/256. I think this is enough and saves the bytes compared to using floats.

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
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

@ansis ansis requested a review from ChrisLoer August 14, 2018 01:49
@ChrisLoer
Copy link
Contributor

This changes makes a lot of sense to me and your implementation looks good. The one thing I'm worried about is text/icon-size overflow. I've seen icon-size overflow at 6554, and now it would overflow at 256. Admittedly, 256 already seems like a big value. In the case I saw a user hitting overflow, they were trying to exponentially size an icon so it matched map scale as you zoomed in. Probably something like a fill layer with a pattern would be a better tool for that particular use case, but when the overflow happens the behavior is really confusing/hard-to-debug. Can we do anything to improve that? Like maybe generate intelligible console errors at expression evaluation time?

@ansis
Copy link
Contributor Author

ansis commented Aug 14, 2018

93a40f9 adds runtime warnings for icon-size and text-size values that are too big

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants