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

use circle-stroke-* paint properties in default theme #719

Closed
wants to merge 1 commit into from

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Dec 6, 2017

This PR replaces the old style of doing circle strokes in GL JS with the newer circle-stroke-* paint properties.

I've bumped GL JS to 0.42 because although circle-stroke-* landed in 0.29.0, a the bug fix "Account for circle-stroke-width in queryRenderedFeatures mapbox/mapbox-gl-js#5514" is important to have.

This does have the unfortunate side affect of not working with older GL JS versions, but anyone who can't upgrade their GL JS can keep using the current version of Draw.

Also nullifies #711.

@andrewharvey
Copy link
Collaborator Author

It looks like theme.js still has styles for the static mode. Should these be moved out into https://github.com/mapbox/mapbox-gl-draw-static-mode ?

@mcwhittemore
Copy link
Contributor

@andrewharvey - it looks like I didn't move the static mode stuff to its own repo https://github.com/mapbox/mapbox-gl-draw-static-mode so we'd need to think about how to do that well. Might be worth ticketing out rather than doing in this PR.

@mcwhittemore
Copy link
Contributor

mcwhittemore commented Dec 6, 2017

@andrewharvey - what gain do we get out of using this new property? Is it faster, less bug prone, more future proof? I ask because requiring mapbox-gl-js>=0.42 is a breaking change and I'd prefer to not make breaking changes unless there is a clear benefit to doing so.

@andrewharvey
Copy link
Collaborator Author

what gain do we get out of using this new property?

Simplicity. I wanted to make the circles larger for an application and find it easier to do when it's using circle-stroke.

Might be worth ticketing out rather than doing in this PR.

Agreed, it should be in a different PR.

@mcwhittemore
Copy link
Contributor

@andrewharvey - thanks for the PR, but I don't think this is a good reason to make a breaking change. I'm happy to add a section to the docs linking to alternative stylesheets if you wanted to publish this version as a module in its own right.

@andrewharvey
Copy link
Collaborator Author

@mcwhittemore no problem. I don't think it's worth a new module for such a minor change, so I'll just leave this branch around so if Draw bumps the required GL JS version, this PR can be reopened.

@mcwhittemore
Copy link
Contributor

Sounds good. Thanks @andrewharvey

@mcwhittemore mcwhittemore deleted the circle-stroke branch April 1, 2018 01:32
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