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

update mapboxgl control system, 0.27.0 fixes #98

Merged
merged 17 commits into from
Nov 18, 2016
Merged

update mapboxgl control system, 0.27.0 fixes #98

merged 17 commits into from
Nov 18, 2016

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Nov 17, 2016

Resolves #94, which opens the door for this plugin to use gl-js 0.27.0.

What changed?

  • no more reliance on global mapboxgl or window.mapboxgl within the plugin
  • Removed internal usage of map.addControl for adding the two geocoders, since we can no longer append controls to custom divs. Since this plugin relied on that, we need to not use the control system internally and just append with appendChild to the proper element for the directions UI
  • Updated some CSS properties since the directions instructions is now populated directly below the controls, instead of in a custom div
  • add circle CI Set up CI #99
  • remove require and just use import Uses both import and require #96

TODO

  • propogate accessToken through to the geocoders. Not sure how to access it at the point of initialization like the directions API does.
  • 👍 👎 on the CSS updates
  • update API & readme

cc @tmcw @tristen for the review

@mapsam
Copy link
Contributor Author

mapsam commented Nov 18, 2016

🤔 hmmm tests aren't going to work on CI since we are relying on WebGL to initialize.

[Error: Failed to initialize WebGL]
not ok - this.painter is undefined

@mapsam
Copy link
Contributor Author

mapsam commented Nov 18, 2016

@tmcw ready for review!


this.subscribedActions();
if (this._map.loaded()) this.mapState()
else this._map.on('load', () => this.mapState());

return el;
Copy link
Contributor

Choose a reason for hiding this comment

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

This updates .onAdd but not .onRemove - I think that'll break map.removeControl, which expects .onRemove to exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmcw Woops, just added onRemove. Good catch!

@mapsam
Copy link
Contributor Author

mapsam commented Nov 18, 2016

@tristen I've updated the style slightly and now instructions are located underneath the controls. How do you feel about this update?

screen shot 2016-11-18 at 2 52 42 pm

@mapsam mapsam merged commit be6109d into master Nov 18, 2016
@mapsam mapsam deleted the icontrol branch November 18, 2016 23:11
@mapsam mapsam mentioned this pull request Nov 18, 2016
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