bug fix embed widget #345

boney-bun · 2017-09-26T20:13:39Z

the previous code still encountered problem with CORS

this solve the problem

the GIF:

lucernae

Thank's for implementing my suggestion.
I think it will be easier to copy paste the embed map code like this in a modal.

There are several comments that can be changed, if you don't mind.

lucernae · 2017-09-27T05:20:30Z

geonode/maps/templates/leaflet_maps/map_embed_widget.html

+            var firstProjection = proj4('EPSG:3857');
+            var secondProjection = proj4('EPSG:4326');
+
+            var point_wsg84 = proj4(firstProjection, secondProjection, [{{ resource.center_x }}, {{ resource.center_y }}]);


the resource has its own srid in resource.srid. You should use this to set the projection. instead of assuming it as EPSG:4326 (use 4326 if it is empty).

lucernae · 2017-09-27T05:37:40Z

geonode/maps/urls.py

@@ -39,13 +39,17 @@
    map_embed = 'map_embed'
    # TODO qlr for geoserver
    map_download_qlr = 'map_download_qlr'
+    # TODO no implementation for geoserver
+    map_embed_widget = 'map_embed_widget'


@boney-bun I'm thinking that for this we can just put map_embed_widget view into core geonode, because the frontend code is independent of either backend. I think it is ok to move map_embed_widget code from qgis_server_views to geonode.maps.views
CC @gubuntu

lucernae · 2017-09-27T05:44:08Z

Also, @boney-bun you might be interested with this:
https://clipboardjs.com
You can provide a button to copy the code content.

codecov · 2017-09-27T06:03:00Z

Codecov Report

Merging #345 into master-qgis_server will decrease coverage by 0.07%.
The diff coverage is 8.1%.

@@                  Coverage Diff                   @@
##           master-qgis_server     #345      +/-   ##
======================================================
- Coverage               49.52%   49.44%   -0.08%     
======================================================
  Files                     267      267              
  Lines                   20453    20488      +35     
  Branches                 2635     2643       +8     
======================================================
+ Hits                    10129    10130       +1     
- Misses                   9698     9732      +34     
  Partials                  626      626

Impacted Files	Coverage Δ
geonode/maps/urls.py	`96.42% <ø> (ø)`	⬆️
geonode/settings.py	`90.27% <100%> (ø)`	⬆️
geonode/maps/views.py	`44.58% <5.55%> (-2.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55483f3...d7b7674. Read the comment docs.

the previous code still encountered problem with CORS this solve the problem fix kartoza#152

embed widget link and add copy button fix kartoza#152

boney-bun · 2017-09-27T16:12:23Z

thanks for your feedback @lucernae
i've made some changes accordingly.
the new UI:

gubuntu · 2017-09-28T03:22:55Z

LGTM except another concern with how we are using Leaflet.

We should not have to use proj4js nor do any of the projection calculations since Leaflet handles 3857 and 4326 under the hood, built in. @boney-bun did you add this? We might have to review how we do it across Kartoza. But for now try to do it the intended Leaflet way in this PR.

lucernae · 2017-09-28T03:47:38Z

@gubuntu @boney-bun Leaflet can't handle crs other than 3857 and 4326. So for some edge cases where the layer uses other crs, we need to convert it in the views (bbox, center, layer projection request to tile or wms)

Gustry · 2017-09-28T04:15:09Z

I'm half following this discussion, but we should stick to tiles stream which are always in 3857

lucernae · 2017-09-28T05:44:09Z

I'm half following this discussion, but we should stick to tiles stream which are always in 3857

Yes, what I mean is, in the django view we need to make sure that everything is converted to correct crs.

Gustry · 2017-09-28T06:11:19Z

Yes, what I mean is, in the django view we need to make sure that everything is converted to correct crs.

Yes, it is. XYZ tiles are designed to be used in 3857 only, whatever the layer CRS.

lucernae · 2017-09-28T06:50:15Z

Yes, it is. XYZ tiles are designed to be used in 3857 only, whatever the layer CRS.

@boney-bun can you confirm that you follow along the discussion?
Maybe there is some misunderstanding, when I said views, it means django view (python code).
Basically we suggests that the conversion happens in django view, instead of the client side.
So leaflet received it on the correct CRS (3857). This includes:

Converting center coordinate to 3857 in map_embed_widget view.
OWS Url returned from maplayer can be WMS or tile url. If it is a local layer, I think we already convert tile url to correct CRS. But not sure about the WMS.

Gustry · 2017-09-28T08:41:52Z

OWS Url returned from maplayer can be WMS or tile url. If it is a local layer, I think we already convert tile url to correct CRS. But not sure about the WMS.

Yes, use the tile url. This URL is always in 3857. And we have caching on tiles, not on WMS. So it's better.

gubuntu · 2017-09-28T09:04:22Z

yes, Geonode/GeoSAFE and QGIS server talk only 3857 or 4326 coordinates for our purposes (and almost all web mapping purposes)

If another client makes a WMS request in another CRS to QGIS server, no problem. But for our Leaflet clients it's only 3857 and 4326.

gubuntu · 2017-09-28T09:30:24Z

@Gustry now that's changing scope...:-)

@boney-bun please just get the Leaflet working with WMS like all our other downloads, with the cruft stripped out. In fact please create a new issue to address stripping out the cruft in ALL our leaflet code that so it's not a blocker for this PR.

Then create another issue to the backlog to add 'Tile layer' as an option for all downloads. But don't start work on it...

boney-bun · 2017-09-28T15:37:05Z

ok, i get lost now.
i have just made a change based on the @lucernae last thread
but, then I read your last comment @gubuntu
cmiiw, it means i should just go with the projection on the frontend instead?
the latest commit moves the projection into views.
let me know what do you think @gubuntu @Gustry @lucernae

gubuntu · 2017-09-29T01:53:16Z

@boney-bun firstly, please reference the new issues I requested above.

You can also make @lucernae's comment into a new issue: it is necessary to compute the bounding box of all the layers in a map.

My comment about removing projection cruft from Leaflet is unrelated and also needs to be done. But address them in separate issues and get this PR merged so we can close #152. The other issues are all about improving under the hood code and technical debt.

boney-bun · 2017-10-18T05:08:01Z

i've made another issue regarding proj4js (#357) and tile layer (#358).
so, i'll merge and close this ticket.

gubuntu · 2017-10-18T06:00:54Z

@boney-bun please reference the issues you created in your comment above

gubuntu · 2017-10-30T07:00:02Z

this is not fixed on the test server. Only give an issue the testing label when it is deployed on testing and able to be tested....

boney-bun · 2017-10-30T08:31:15Z

i have just tested on testing.
this issue has already been addressed on testing.

the steps are:

login
click on Embed Map then Embed Widget Link
click on Copy to Clipboard
make a new file and paste all the codes on it
save it. make sure the file name ends with .html
open the file.
You should be able to see the corresponding map.

below is the illustration:

gubuntu · 2017-10-30T08:41:26Z

not working for me, see my comment in the issue (Firefox)

boney-bun · 2017-10-30T08:51:06Z

ok, i managed to replicate your issue with firefox now.
let me investigate it.

boney-bun · 2017-10-31T12:16:33Z

#376 should fix the bug when using firefox

boney-bun requested a review from lucernae September 26, 2017 20:13

boney-bun added the in progress label Sep 26, 2017

lucernae reviewed Sep 27, 2017

View reviewed changes

boney-bun added 3 commits September 27, 2017 17:38

bug fix embed widget

33ffa14

the previous code still encountered problem with CORS this solve the problem fix kartoza#152

fix travis error

c8f92c9

embed widget link for qgis-server and geoserver backend

d5fdb98

embed widget link and add copy button fix kartoza#152

boney-bun force-pushed the bug-fix-embed-widget-#152 branch from 4a70eec to d5fdb98 Compare September 27, 2017 16:08

Merge branch 'master-qgis_server' into bug-fix-embed-widget-kartoza#152

f3ddf9c

refactor map projection on the frontend into django views

6149dba

Merge branch 'master-qgis_server' into bug-fix-embed-widget-kartoza#152

3a4cf9d

This was referenced Oct 13, 2017

leaflet page should free from using proj4js #357

Open

add other format options to embed content #358

Open

Merge branch 'master-qgis_server' into bug-fix-embed-widget-kartoza#152

d7b7674

boney-bun merged commit 8ae1950 into kartoza:master-qgis_server Oct 18, 2017

boney-bun removed the in progress label Oct 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug fix embed widget #345

bug fix embed widget #345

boney-bun commented Sep 26, 2017

lucernae left a comment

lucernae Sep 27, 2017

lucernae Sep 27, 2017

lucernae commented Sep 27, 2017

codecov bot commented Sep 27, 2017 •

edited

Loading

boney-bun commented Sep 27, 2017

gubuntu commented Sep 28, 2017

lucernae commented Sep 28, 2017

Gustry commented Sep 28, 2017

lucernae commented Sep 28, 2017

Gustry commented Sep 28, 2017

lucernae commented Sep 28, 2017 •

edited

Loading

Gustry commented Sep 28, 2017

gubuntu commented Sep 28, 2017

gubuntu commented Sep 28, 2017

boney-bun commented Sep 28, 2017

gubuntu commented Sep 29, 2017

boney-bun commented Oct 18, 2017 •

edited

Loading

gubuntu commented Oct 18, 2017

gubuntu commented Oct 30, 2017

boney-bun commented Oct 30, 2017

gubuntu commented Oct 30, 2017

boney-bun commented Oct 30, 2017

boney-bun commented Oct 31, 2017

bug fix embed widget #345

bug fix embed widget #345

Conversation

boney-bun commented Sep 26, 2017

lucernae left a comment

Choose a reason for hiding this comment

lucernae Sep 27, 2017

Choose a reason for hiding this comment

lucernae Sep 27, 2017

Choose a reason for hiding this comment

lucernae commented Sep 27, 2017

codecov bot commented Sep 27, 2017 • edited Loading

Codecov Report

boney-bun commented Sep 27, 2017

gubuntu commented Sep 28, 2017

lucernae commented Sep 28, 2017

Gustry commented Sep 28, 2017

lucernae commented Sep 28, 2017

Gustry commented Sep 28, 2017

lucernae commented Sep 28, 2017 • edited Loading

Gustry commented Sep 28, 2017

gubuntu commented Sep 28, 2017

gubuntu commented Sep 28, 2017

boney-bun commented Sep 28, 2017

gubuntu commented Sep 29, 2017

boney-bun commented Oct 18, 2017 • edited Loading

gubuntu commented Oct 18, 2017

gubuntu commented Oct 30, 2017

boney-bun commented Oct 30, 2017

gubuntu commented Oct 30, 2017

boney-bun commented Oct 30, 2017

boney-bun commented Oct 31, 2017

codecov bot commented Sep 27, 2017 •

edited

Loading

lucernae commented Sep 28, 2017 •

edited

Loading

boney-bun commented Oct 18, 2017 •

edited

Loading