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 leaflet.restoreview.js plugin to save and restore last user's map… #49

Merged
merged 2 commits into from
Jul 26, 2016

Conversation

bagage
Copy link
Collaborator

@bagage bagage commented Jul 20, 2016

… position between sessions (#48)

I am not JS developer so please tell me if something is wrong with my PR. Thanks @nrenner!

@bagage bagage mentioned this pull request Jul 20, 2016
2 tasks
@bagage
Copy link
Collaborator Author

bagage commented Jul 21, 2016

Added another patch for 2nd part of #48. I added reference to Awesome font in index.html, but maybe you wish not to use it?
Again any comment's welcome, I am a truly newbie here!

@nrenner
Copy link
Owner

nrenner commented Jul 21, 2016

Thanks for the PR! I do have comments, but don't worry, still learning myself. And I should have more documentation.

The build setup using Bower and Gulp is still a bit rough and not ideal. Also already outdated again, as people seem to have moved on to Webpack nowadays. But what I'm trying to achieve is that all dependencies are referenced and downloaded through Bower, so that the origin and the version used are documented and that managing and updating the depencencies gets a bit easier.

leaflet.restoreview:

I would prefer js/plugin/leaflet.restoreview.js to be added and downloaded via Bower although its an overhead in this case. The dir name plugin is a bit misleading here, intended for own configuration and extension of external plugins only.

As it has no bower.json and is not registered with bower (https://bower.io/search/), I think you can reference the repo and add it to the bower.json with

bower install makinacorpus/Leaflet.RestoreView#master --save

Then add the file to include (leaflet.restoreview.js) to the "overrides" section. Check if included with gulp debug.

leaflet.locatecontrol

Yes, I would prefer not to load another fontset, as we already have Glyphicons from Bootstrap, but Glyphicons is a bit limited and seems not to have a spinner. Not sure about the marker icon, maybe fa-location-arrow or something similar to glyphicon glyphicon-screenshot (but more GPSy) instead, what do you think?

But I might as well merge the commit as it is and think about that later, as I want to add more icons anyway and am not decided on the icon-/fontset yet.

@bagage bagage force-pushed the master branch 2 times, most recently from 1b23c37 to 51abe1c Compare July 21, 2016 17:06
@bagage
Copy link
Collaborator Author

bagage commented Jul 21, 2016

Thanks for your quick feedbacks!

  • I moved leaflet.restoreview to bower
  • I removed Awesome font dependency and instead use glyphicon-refresh and glyphicon-screenshot icons instead. They are not perfect but I guess it's OK for now. I also added a bower overrides section for locatecontrol plugin but I am unsure about it; could you take a look at it please?

@nrenner
Copy link
Owner

nrenner commented Jul 26, 2016

Thanks for the update. Yes, the icons will do for now.

The bower override for locatecontrol isn't necessary, as the project already has a bower.json with a proper main section.

Could you please remove that again, then I think we're ready to merge.

@bagage
Copy link
Collaborator Author

bagage commented Jul 26, 2016

OK I removed it, I can't wait to use these on brouter-web! 😄

@nrenner nrenner merged commit c9cd085 into nrenner:master Jul 26, 2016
@bagage bagage mentioned this pull request Sep 6, 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