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 multiple features per ID per tile #132

Merged
merged 2 commits into from
Jul 26, 2019
Merged

Allow multiple features per ID per tile #132

merged 2 commits into from
Jul 26, 2019

Conversation

brendan-ward
Copy link
Contributor

Like #65, I was running into a case where only some features were getting updated with setFeatureStyle and resetFeatureStyle.

The underlying reason is addressed in this PR: there are multiple features that have the same ID in a given tile, but only the last encountered was stored with that ID (in renderer._features[id]).

I changed this to allow an array instead of a single feature and updated setFeatureStyle and resetFeatureStyle to use this. It fixed my particular case.

(not sure what is going on here with whitespace, it looks right in my editor. Sorry for any formatting issues)

Copy link

@traustid traustid left a comment

Choose a reason for hiding this comment

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

This seems to be solving the problem I'm having with multiple features with the same id (or one feature split up to multiple polygons) not updating properly with setFeatureStyle.

@zogs
Copy link

zogs commented Apr 5, 2018

This fix solved my problem as well, this should be reviewed and added to future release.

@brendan-ward
Copy link
Contributor Author

This is ready for re-review (conflicts against master now resolved).

Copy link

@revatim revatim left a comment

Choose a reason for hiding this comment

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

This works please merge this code :: it is needed functionality

Copy link
Member

@IvanSanchez IvanSanchez left a comment

Choose a reason for hiding this comment

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

Several people have tested this, so LGTM.

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.

5 participants