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

Map class cleanup #7060

Merged
merged 3 commits into from
Aug 1, 2018
Merged

Map class cleanup #7060

merged 3 commits into from
Aug 1, 2018

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 1, 2018

A cleanup sweep for the Map class. See individual commits. Fixes #2741. I don't think there's much more logic left to move downstream unless we break the API.

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

@mourner mourner requested a review from jfirebaugh August 1, 2018 10:14
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

LGTM. However I've always interpreted #2741 as being about the larger, API breaking change to make the runtime styling API more like native, where style and layer types are public and not everything needs to route through Map.

@@ -1782,13 +1782,13 @@ test('Style#queryRenderedFeatures', (t) => {
style._updateSources(transform);

t.test('returns feature type', (t) => {
const results = style.queryRenderedFeatures([{column: 1, row: 1, zoom: 1}], {}, transform);
Copy link
Contributor

Choose a reason for hiding this comment

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

First argument was supposed to be {viewport, worldCoordinate}, so apparently this was just working by accident before.

@mourner
Copy link
Member Author

mourner commented Aug 1, 2018

@jfirebaugh yeah, but that interpretation is only mentioned as a small comment at the bottom of the issue. So I think we should make a new issue focused on the API change.

@mourner mourner merged commit 7cdb197 into master Aug 1, 2018
@mourner mourner deleted the map-cleanup branch August 1, 2018 16:01
@mourner mourner mentioned this pull request Aug 1, 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.

2 participants