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

Fix missing frontend dependencies for leaflet. #3128

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

lucernae
Copy link
Contributor

@lucernae lucernae commented Jun 12, 2017

Related with issue #3116.
@simod can you please review this, because I'm not too familiar with bower and gruntfile.
I tried to follow the existing pattern and code practice in Geonode's staticfiles, but you can comment on it if you see something is not right.
@afabiani you might want to review this too, because I saw you made latest changes in layer_leaflet_map.html, so you are probably more familiar in this.

Changelist

Included to bower.json

  • Leaflet opacity control plugin
  • Leaflet measure plugin
  • Leaflet navbar plugin

Modify gruntfile tasks

  • Modify css url images regex pattern for replace task
  • Create another minified file for leaflet plugins
  • Refactor some js filename to match in what used in the template when
    DEBUG_STATIC is true
  • Add task replace for task production

Related changes

  • Included images from library from grunt copy:development task
  • New minified js for assets.min.js and leaflet-plugins.min.js from
    production task
  • New minified css for assets.min.css and leaflet-plugins.min.css
  • Moved some pngs from lib/css folder to lib/img from the new replace task

Testing Environment

Django settings

Using DEBUG_STATIC = True

DEBUG_STATIC = True
LAYER_PREVIEW_LIBRARY = 'leaflet'
# can be seen in geonode/settings.py
LEAFLET_CONFIG = {
    'TILES': [
        # Find tiles at:
        # http://leaflet-extras.github.io/leaflet-providers/preview/

        # Stamen toner lite.
        ('Watercolor',
         'http://{s}.tile.stamen.com/watercolor/{z}/{x}/{y}.png',
         'Map tiles by <a href="http://stamen.com">Stamen Design</a>, \
         <a href="http://creativecommons.org/licenses/by/3.0">CC BY 3.0</a> \
         &mdash; Map data &copy; \
         <a href="http://openstreetmap.org">OpenStreetMap</a> contributors, \
         <a href="http://creativecommons.org/licenses/by-sa/2.0/"> \
         CC-BY-SA</a>'),
        ('Toner Lite',
         'http://{s}.tile.stamen.com/toner-lite/{z}/{x}/{y}.png',
         'Map tiles by <a href="http://stamen.com">Stamen Design</a>, \
         <a href="http://creativecommons.org/licenses/by/3.0">CC BY 3.0</a> \
         &mdash; Map data &copy; \
         <a href="http://openstreetmap.org">OpenStreetMap</a> contributors, \
         <a href="http://creativecommons.org/licenses/by-sa/2.0/"> \
         CC-BY-SA</a>'),
    ],
    'PLUGINS': {
        'esri-leaflet': {
            'js': 'lib/js/esri-leaflet.js',
            'auto-include': True,
        },
        'leaflet-fullscreen': {
            'css': 'lib/css/leaflet.fullscreen.css',
            'js': 'lib/js/Leaflet.fullscreen.min.js',
            'auto-include': True,
        },
        'leaflet-opacity': {
            'css': 'lib/css/Control.Opacity.css',
            'js': 'lib/js/Control.Opacity.js',
            'auto-include': True,
        },
        'leaflet-navbar': {
            'css': 'lib/css/Leaflet.NavBar.css',
            'js': 'lib/js/Leaflet.NavBar.js',
            'auto-include': True,
        },
        'leaflet-measure': {
            'css': 'lib/css/leaflet-measure.css',
            'js': 'lib/js/leaflet-measure.js',
            'auto-include': True,
        },
    },
    'SRID': 3857,
    'RESET_VIEW': False
}

Using DEBUG_STATIC = False (default production environment)

DEBUG_STATIC = False
LAYER_PREVIEW_LIBRARY = 'leaflet'
# can be seen in geonode/settings.py
LEAFLET_CONFIG = {
    'TILES': [
        # Find tiles at:
        # http://leaflet-extras.github.io/leaflet-providers/preview/

        # Stamen toner lite.
        ('Watercolor',
         'http://{s}.tile.stamen.com/watercolor/{z}/{x}/{y}.png',
         'Map tiles by <a href="http://stamen.com">Stamen Design</a>, \
         <a href="http://creativecommons.org/licenses/by/3.0">CC BY 3.0</a> \
         &mdash; Map data &copy; \
         <a href="http://openstreetmap.org">OpenStreetMap</a> contributors, \
         <a href="http://creativecommons.org/licenses/by-sa/2.0/"> \
         CC-BY-SA</a>'),
        ('Toner Lite',
         'http://{s}.tile.stamen.com/toner-lite/{z}/{x}/{y}.png',
         'Map tiles by <a href="http://stamen.com">Stamen Design</a>, \
         <a href="http://creativecommons.org/licenses/by/3.0">CC BY 3.0</a> \
         &mdash; Map data &copy; \
         <a href="http://openstreetmap.org">OpenStreetMap</a> contributors, \
         <a href="http://creativecommons.org/licenses/by-sa/2.0/"> \
         CC-BY-SA</a>'),
    ],
    'PLUGINS': {
        'leaflet-plugins': {
            'js': 'lib/js/leaflet-plugins.min.js',
            'css': 'lib/css/leaflet-plugins.min.css',
            'auto-include': True,
        }
    },
    'SRID': 3857,
    'RESET_VIEW': False
}

Tested views

Listed here are the views that I check (manually) to see that css and js files were resolved correctly

  • Layer list view ( /layers )
  • Layer detail view ( /layers/ )

Screenshots

fix_leaflet_view

Notes

There is also a missing logo at the bottom right. I'm not sure what logo is this. Maybe geonode logo?

<script src="/static/lib/navbar/Leaflet.NavBar.js"></script>

<link rel="stylesheet" href="/static/lib/measure/leaflet.measure.css" />
<script src="/static/lib/measure/leaflet.measure.js"></script>
Copy link
Contributor Author

@lucernae lucernae Jun 12, 2017

Choose a reason for hiding this comment

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

Deleted these so it can be included from {% leaflet_js %} tags

.gitignore Outdated
geonode/static/lib/css/bootstrap-tokenfield.css
geonode/static/lib/js/assets.min.js
geonode/static/lib/js/leaflet-plugins.min.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included to follow .gitignore pattern that minified js and css should not automatically be committed, because it will change for every grunt production

@@ -166,6 +151,7 @@
onAdd: function(map) {
var img = L.DomUtil.create('img');

// I'm not sure what is this logo supposed to be?
img.src = '../static/img/logo.png';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is for geonode logo?
If that is the case, we can point to geonode logo's static url.

Copy link

Choose a reason for hiding this comment

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

As a gut check, I would have thought that it should be /static/geonode/img/logo.png from ./geonode/static/geonode/img/logo.png. I would personally play with the value and test to verify this, if I had time

if(L.Control.hasOwnProperty('opacitySlider')) {
//adjust opacity
var opacitySlider = new L.Control.opacitySlider();
map.addControl(opacitySlider);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow fullscreen plugin pattern. Only add when the plugin is resolved.

L.control.navbar().addTo(map);
if(L.control.hasOwnProperty('navbar')){
//adjust info
L.control.navbar().addTo(map);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow fullscreen plugin pattern. Only add when the plugin is resolved.

'css': 'lib/css/leaflet-measure.css',
'js': 'lib/js/leaflet-measure.js',
'auto-include': True,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add these 3 plugins with each css and js when DEBUG_STATIC = True

'leaflet-plugins': {
'js': 'lib/js/leaflet-plugins.min.js',
'css': 'lib/css/leaflet-plugins.min.css',
'auto-include': True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use minified plugins, when not in DEBUG_STATIC

@@ -27,6 +27,9 @@
"angular-leaflet-directive": "0.7.9",
"zeroclipboard": "2.1.5",
"leaflet-fullscreen": "https://github.com/Leaflet/Leaflet.fullscreen.git",
"leaflet-opacity": "https://github.com/lizardtechblog/Leaflet.OpacityControls.git#master",
"leaflet-navbar": "1.0.1",
"leaflet-measure": "2.1.7",
Copy link
Contributor Author

@lucernae lucernae Jun 12, 2017

Choose a reason for hiding this comment

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

Added leaflet plugins reference in bower.
This will install the plugin to .components when doing bower install

@@ -83,8 +83,9 @@ module.exports = function(grunt) {
'jquery-tree-multiselect/dist/jquery.tree-multiselect.min.css',
'bootstrap/dist/css/bootstrap.min.css',
'leaflet-fullscreen/dist/leaflet.fullscreen.css',
'leaflet-fullscreen/dist/[email protected]',
'leaflet-fullscreen/dist/fullscreen.png',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move fullscreen*.png to lib/img

@@ -100,8 +101,15 @@ module.exports = function(grunt) {
dest: 'lib/img',
src: [
'bootstrap/img/*.png',
'select2*.png', 'select2-spinner.gif',
'select2/select2*.png',
'select2/select2*.gif',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use proper select2 namespace/folder.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 46.098% when pulling 114d689a6c977c7fa6aaad80238dee2a67d0df38 on lucernae:fix_leaflet_view-upstream into d0f5d72 on GeoNode:master.

'jquery-ui/ui/minified/jquery-ui.custom.min.js',
'raty/lib/jquery.raty.js',
'jquery-waypoints/waypoints.js',
'jquery-ui/ui/jquery-ui.custom.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to change these, because it is what being referenced in base.html when DEBUG_STATIC = True.
The developers probably need the unminified version for debugging.

}, {
from: /(png|gif|jpg)+(\)|'\)|"\))/g,
to: '$1\')'
}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the pattern in the hope that it will be more readable and understandable.
I also had some issues with the previous pattern didn't replace the text properly for Leaflet.NavBar.css

'lib/css/leaflet.fullscreen.css',
'lib/css/Leaflet.NavBar.css',
'lib/css/leaflet-measure.css',
'lib/css/Control.Opacity.css'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

minified all used leaflet-plugins.

'lib/js/Leaflet.NavBar.js',
'lib/js/leaflet-measure.js',
'lib/js/Control.Opacity.js'
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

minified all used leaflet-plugins.

@@ -278,6 +355,6 @@ module.exports = function(grunt) {
grunt.registerTask('default', ['jshint', 'less:development', 'concat:bootstrap', 'copy', 'replace']);

// build production
grunt.registerTask('production', ['jshint', 'less:production', 'concat:bootstrap', 'copy', 'cssmin', 'uglify' ]);
grunt.registerTask('production', ['jshint', 'less:production', 'concat:bootstrap', 'copy', 'replace', 'cssmin', 'uglify' ]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Include replace task for production task

@simod
Copy link
Member

simod commented Jun 13, 2017

Thanks @lucernae this is a good description. Usually we try to avoid the js third party source files like this opacity and measure js files as they can come using bower install. Though is good to ship them minified in the leaflet.plugins.min.js.

@lucernae
Copy link
Contributor Author

In that case, I can exclude these 3rd party source files from commit and only include the minified version, is that ok? I also think that's a good idea, it was included in bower.json afterall.

For the missing logo, if that's ok, I can set it to geonode url (@harts-boundless suggestion).

@simod
Copy link
Member

simod commented Jun 13, 2017

+1!

**Included to bower.json**

* Leaflet opacity control plugin
* Leaflet measure plugin
* Leaflet navbar plugin

**Modify gruntfile tasks**

* Modify css url images regex pattern for replace task
* Create another minified file for leaflet plugins
* Refactor some js filename to match in what used in the template when
  DEBUG_STATIC is true
* Add task replace for task production

**Related changes**

* Included images from library from grunt copy:development task
* New minified js for assets.min.js and leaflet-plugins.min.js from
  production task
* New minified css for assets.min.css and leaflet-plugins.min.css
* Moved some pngs from lib/css folder to lib/img from the new replace task
* Include Geonode Logo
@lucernae
Copy link
Contributor Author

Changes:

  • Put Geonode logo as default:

screen shot 2017-06-14 at 16 13 20

  • Exclude leaflet plugins individual source file. But still include minified css and js for combined leaflet plugins and image files.

This is now ready to be merged, if it's ok.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 46.098% when pulling 473b7e7 on lucernae:fix_leaflet_view-upstream into d0f5d72 on GeoNode:master.

@afabiani
Copy link
Member

It seems good to me.

@simod simod merged commit 4ce5393 into GeoNode:master Jun 15, 2017
@lucernae lucernae deleted the fix_leaflet_view-upstream branch June 15, 2017 07:45
lucernae added a commit to kartoza/geonode that referenced this pull request Jun 15, 2017
**Included to bower.json**

* Leaflet opacity control plugin
* Leaflet measure plugin
* Leaflet navbar plugin

**Modify gruntfile tasks**

* Modify css url images regex pattern for replace task
* Create another minified file for leaflet plugins
* Refactor some js filename to match in what used in the template when
  DEBUG_STATIC is true
* Add task replace for task production

**Related changes**

* Included images from library from grunt copy:development task
* New minified js for assets.min.js and leaflet-plugins.min.js from
  production task
* New minified css for assets.min.css and leaflet-plugins.min.css
* Moved some pngs from lib/css folder to lib/img from the new replace task
* Include Geonode Logo
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.

4 participants