socib / Leaflet.TimeDimension

Add time dimension capabilities on a Leaflet map.
MIT License
435 stars 139 forks source link

Bower dependencies too restrictive #21

Closed r1m closed 8 years ago

r1m commented 8 years ago

After #20, which is a good thing, I was about to do it myself. May I suggest :

"dependencies": {
    "iso8601-js-period": "*",
    "leaflet": "~0.7.4 || ~1.0.0",
    "jquery": "1.x || 2.x"
    "jquery-ui": ">=1.10"
  }

Because you use almost use nothing from jquery ($.ajax), you don't need a specific version. You also support leaflet 1.0

r1m commented 8 years ago

I'm also wondering why ignore list contains "src" folder.

bielfrontera commented 8 years ago

Hi @ram-one, ok, I'm changing the dependencies.

I think we should also change main to:

  "main": [
    "dist/leaflet.timedimension.src.js",
    "dist/leaflet.timedimension.control.css",
  ],

I'm not sure about the src folder. I've checked some projects, and some of them include this folder (ex: jQuery) and others don't (ex: Leaflet). Initially, you shouldn't use the files of src folder, as you have them concatenated and minified at dist.

Any suggestion @fox91?

r1m commented 8 years ago

Yes, main is also wrong because bower will ignore src folder when downloading the package so automated tool that uses the main declaration won't find theses.

Including src allows custom builds. For example I don't want the player control because I use mine. If the folder is in ignore list, it is simply not donwloaded.

fox91 commented 8 years ago

Hi @bielfrontera, I also looked at how it is implemented in other projects. I think the src folder is essential for the development phase (git clone or npm) but not in the integration phase (bower), following the philosophy of Leaflet. In any case it is a decision they make the developers of each project, as there are no real guidelines on the folders to skip. Include the dist/ folder in the main is definitely correct.

Dependencies suggested by @ram-one seem appropriate.

fox91 commented 8 years ago

Hi @bielfrontera, I have included some changes in the pull request #22. If you proceed with the merge you must first regenerate the dist/ folder.

bielfrontera commented 8 years ago

Ok! Updated the version number to 0.1.7 and built with the enhancement for leaflet 1.0.