[WIP]Embed feature in demo page #296

crisner · 2019-12-09T11:36:10Z

Fixes #252 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

crisner · 2019-12-09T11:40:57Z

Screenshot of the feature:

crisner · 2019-12-09T11:46:49Z

I have added the embed feature only to the layers.js file as it was specifically for the demo page. Should this be implemented in AllLayers.js as well?

Also, when I was referring to the embed feature implemented in plots2 I noticed that the URL has query strings. Should that be implemented here as well?

crisner · 2019-12-09T11:49:02Z

Should the embed button follow the style guide and have a bootstrap styled button or should it be similar to the zoom controls in the map?

crisner · 2019-12-09T11:55:43Z

Embed code in use at the bottom of the page --> https://codepen.io/crisren/full/abzdJPV

sagarpreet-chadha

@jywarren @ebarry
Can you confirm if the position of embed button is fine? Thanks!

sagarpreet-chadha · 2019-12-09T14:53:16Z

src/util/embedControl.js

+
+    generateCode: function() {
+        var params = this.getUrlHashParameters();
+        var code = '<iframe style="border:none;" width="100%" height="900px" src="//publiclab.github.io/leaflet-environmental-layers/example/#' + params.zoom + '/' + params.lat + '/' + params.lng + '/' + params.layers +'"></iframe>';


Hey @crisner ,
Instead of this URL, let's use the one i created in PL:
https://publiclab.org/map/

I tried loading the mentioned URL but I am getting a 404 page.

sagarpreet-chadha · 2019-12-09T14:54:14Z

src/util/embedControl.js

+        return code;
+    },
+
+    onRemove: function(map) {}


Can you verify if on calling remove function, the button gets removed?

I didn't implement a remove function thinking there wasn't a need for it. I shall check it.

The button gets removed using removeControl(control) on the map object. It looks like the onRemove method works as expected.

Okay cool! You never know what use-case the user may need :)

sagarpreet-chadha · 2019-12-09T14:54:53Z

example/layers.js

@@ -232,6 +232,8 @@
    var hash = new L.Hash(map, allMapLayers);
    var leafletControl = new L.control.layers(baseMaps,overlayMaps);
    leafletControl.addTo(map);
-
+


AWESOME!!!
Let's add this to all HTML demo files :)

sagarpreet-chadha · 2019-12-09T14:56:45Z

example/index.html

@@ -38,6 +38,8 @@
  <script src="https://cdnjs.cloudflare.com/ajax/libs/OverlappingMarkerSpiderfier-Leaflet/0.2.6/oms.min.js"></script>

  <script src="lib/glify.js"></script>
+
+  <script src="https://kit.fontawesome.com/92fa0312fd.js" crossorigin="anonymous"></script>


Can we download this font-awesome file and add that to LEL repo in /images folder so that the button image is there even if we are offline?

And then remove this script also. Thanks!

We could include it using NPM and then it'd be in node_modules?

jywarren · 2019-12-09T22:13:27Z

Regarding the styling, i guess it might work best as a native Leaflet styled button, like the full screen buttons on https://publiclab.org/wiki/unearthing-pvd for example. Once you try it out can you share a screenshot?

Thank you! Looking awesome!

crisner · 2019-12-10T05:02:48Z

Thank you for the feedback @sagarpreet-chadha and @jywarren! I shall make the changes as suggested.

crisner · 2019-12-10T13:40:03Z

@jywarren, here's the screenshot of the embed button I have now. What do you think?

jywarren · 2019-12-10T15:44:03Z

Oh that's beautiful! Great job! I was thinking that, in the online gh-pages demo, we may want a page that explicitly says something like "Pan & zoom the map below and select layers to display, then use the `</>` button to generate an embed code you can use on your own website." -- this could just be a strip of text at the top, you know? That way the gh-pages demo is a utility as well as a demo. Later, we could actually offer a choice of embed types - HTML <iframe> or plots2 shortcode ([map:______] style) on this example page. What do you think?

…

On Tue, Dec 10, 2019 at 8:40 AM Renisha Christie. A < ***@***.***> wrote: @jywarren <https://github.com/jywarren>, here's the screenshot of the embed button I have now. What do you think? [image: 127 0 0 1_5500_example_index html (2)] <https://user-images.githubusercontent.com/29401459/70534117-61df2800-1b80-11ea-9aad-5387a7c00108.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296?email_source=notifications&email_token=AAAF6J4EDMYCACKJZCXFNYDQX6L3JA5CNFSM4JYHWOY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGPIGFA#issuecomment-564036372>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J3X7M6PBFRYCOV4SCDQX6L3JANCNFSM4JYHWOYQ> .

crisner · 2019-12-10T16:01:48Z

This is what I have so far:

Assuming the URL needed to be formatted as:
http://localhost:3000/map/Unearthing,eonetFiresLayer,odorreport,asian#lat=30.600093873550072&lon=52.03125&zoom=3
I tried to implement the hash but with the formatted code by modifying the fullHash library. -> https://github.com/crisner/leaflet-fullUrlHash. I have the following format now:
http://127.0.0.1:5500/example/index.html#lat=43.00&lon=-83.00&zoom=3&layers=BL1
Added fontawesome to node modules
Added native leaflet style to embed icon

Issues I am facing:

When I click on the prompt buttons, generated more than once after loading the page, for some reason the location hash gets reset. I will be looking into this tomorrow.

jywarren · 2019-12-10T16:17:24Z

This sounds great. Could we split out the domain name as a settable option defaulting to the gh-pages https://publiclab.github.io/leaflet-environmental-layers/ but overridable when LEL is initialized with something like options.hostname or options.root_url ? I guess it's more complicated than that but maybe it's worth spinning out a new issue to follow-up on this idea. Sorry to throw more and more into this PR! We can focus on solving a narrower task here and defer any extra stuff for a separate issue/PR; that's a best practice anyways! Thanks!!

…

On Tue, Dec 10, 2019 at 11:06 AM Renisha Christie. A < ***@***.***> wrote: This is what I have so far: - Assuming the URL needed to be formatted as: http://localhost:3000/map/Unearthing,eonetFiresLayer,odorreport,asian#lat=30.600093873550072&lon=52.03125&zoom=3 I tried to implement the hash but with the formatted code by modifying the fullHash library. -> https://github.com/crisner/leaflet-fullUrlHash. I have the following format now: http://127.0.0.1:5500/example/index.html#lat=43.00&lon=-83.00&zoom=3&layers=BL1 - Added fontawesome to node modules - Added native leaflet style to embed icon Issues I am facing: - When I click on the prompt buttons, generated more than once after loading the page, for some reason the location hash gets reset. I will be looking into this tomorrow. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296?email_source=notifications&email_token=AAAF6J24CUGB2U3PFRLPRKLQX64OZA5CNFSM4JYHWOY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGPYLOY#issuecomment-564102587>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J63UQY4VSPTYKWXQRDQX64OZANCNFSM4JYHWOYQ> .

crisner · 2019-12-10T16:25:28Z

Thanks, @jywarren! I like the idea of spinning it out as a new issue. That way it is easier for me to focus.

jywarren · 2019-12-10T16:51:36Z

Super!

…

On Tue, Dec 10, 2019, 11:25 AM Renisha Christie. A ***@***.***> wrote: Thanks, @jywarren <https://github.com/jywarren>! I like the idea of spinning it out as a new issue. That way it is easier for me to focus. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296?email_source=notifications&email_token=AAAF6J3BR57T4QSDAIC5T5DQX67HTA5CNFSM4JYHWOY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGP3CUI#issuecomment-564113745>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J7QCGNFE2NJI3APG4LQX67HTANCNFSM4JYHWOYQ> .

crisner · 2019-12-11T13:52:35Z

Issues I am facing:

When I click on the prompt buttons, generated more than once after loading the page, for some reason the location hash gets reset. I will be looking into this tomorrow.

Fixed this issue.
I tried changing this url:
http://127.0.0.1:5500/example/index.html#lat=43.00&lon=-83.00&zoom=3&layers=BL1
to http://localhost:3000/example/index.html/BL1#lat=-83.00&lon=-83.00&zoom=3

I am getting a cannot GET error message for /BL1. I am assuming it is because there is no such path on the front end. So, presently the URL is http://127.0.0.1:5500/example/index.html#lat=43.00&lon=-83.00&zoom=3&layers=BL1
I have added the embed control to other pages as suggested. This works locally but the actual links point to a 404 page.

These files have the permalinks issue #6889 for which we have a PR Fixes map permalinks overwrite issue on plots2 #293

crisner · 2019-12-11T13:56:39Z

Looking forward to your feedback on the changes @sagarpreet-chadha. :) I would also like your feedback on a couple of issues I have mentioned in my previous comment.

crisner · 2019-12-11T13:58:56Z

Should I also commit the package-lock.json file?

jywarren · 2019-12-11T16:02:21Z

Just quickly, yes please do commit it, as it often includes updates or adjustments. We can pull it out if there's anything too weird in there :-)

Do you know how to test by setting up your own gh-pages branch? This is a good way to check that it'll work the same way without needing to first overwrite the publiclab gh-pages. But you do need to keep a separate branch with gh-pages with your node_modules all added, and commit and push them only to your own gh-pages branch. This gets a little complicated, but it's great for showing off a new version so it can be tested out before being merged!

sagarpreet-chadha · 2019-12-11T16:51:52Z

Awesome work so far!!!
@crisner ...do tell if you need help setting up gh-pages in your forked repo 😄 .

crisner · 2019-12-11T16:56:52Z

I have set up gh-pages before. But it has been a while. I'll give it a go and update my progress here.

sagarpreet-chadha · 2019-12-11T17:03:46Z

Awesome!!!

…

On Wed, Dec 11, 2019 at 10:26 PM Renisha Christie. A < ***@***.***> wrote: I have set up gh-pages before. But it has been a while. I'll give it a go and update my progress here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296?email_source=notifications&email_token=ADSCRRJFWYC4O2YIBMGY6U3QYELVJA5CNFSM4JYHWOY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGT2XMA#issuecomment-564636592>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSCRRKSI7GJKJJCQIU3GOLQYELVJANCNFSM4JYHWOYQ> .

crisner · 2019-12-12T11:49:42Z

It was harder than I expected to figure things out but I have managed to set up a working gh-pages from a 'development' branch on my forked repo.

crisner · 2019-12-12T12:11:38Z

I am also getting the following warning from GitHub in my mail,

The page build completed successfully, but returned the following warning for the gh-pages branch:

It looks like you're using GitHub Pages to distribute binary files. We strongly suggest that you use releases to ship projects on GitHub. Releases are GitHub's way of packaging and providing software to your users. You can think of it as a replacement to using downloads to provide software. We found the following file(s) which may be a good candidate for releases: node_modules/phantomjs-prebuilt/lib/phantom/bin/phantomjs.exe.

crisner · 2019-12-13T11:00:14Z

The active baselayer appears unselected in the map's layer control which affects the url hash as well.
Screenshot of the issue:

I started another branch from the changes in this PR as the base and created issue #299 and linked PR #300 to it. Please let me know if this is not the right way to go about adding more changes in a separate PR. Thanks!

sagarpreet-chadha

just a small change and this looks good!!!

sagarpreet-chadha · 2019-12-14T08:56:13Z

src/AllLayers.js

@@ -121,15 +121,18 @@ L.LayerGroup.environmentalLayers = L.LayerGroup.extend(
 	           }

 	           if(this.options.embed) {


Redundant code?

sagarpreet-chadha · 2019-12-14T08:57:52Z

src/util/embedControl.js

+        return code;
+    },
+
+    onRemove: function(map) {}


Okay cool! You never know what use-case the user may need :)

sagarpreet-chadha · 2019-12-14T09:03:23Z

example/lib/leaflet-fullUrlHash.js

+	var HAS_HASHCHANGE = (function() {
+		var doc_mode = window.documentMode;
+		return ('onhashchange' in window) &&
+			(doc_mode === undefined || doc_mode > 7);


I think we need to add this to a separate folder outside example. Let's make one lib folder at the root level and change the script tag above.

Should I move the whole lib folder to the root or just the fullUrlHash.js file?

sagarpreet-chadha · 2019-12-14T09:11:47Z

Continued to #300 (review)

sagarpreet-chadha · 2019-12-15T15:37:44Z

Yes the whole lib folder, but we should make note that we will have to change script src for glify.js in plots2 then. Thanks!

crisner added 2 commits December 9, 2019 14:42

Add embed feature

fdeac86

Add an options parameter to embed control

291b693

crisner requested a review from sagarpreet-chadha December 9, 2019 11:49

sagarpreet-chadha requested changes Dec 9, 2019

View reviewed changes

crisner added 3 commits December 10, 2019 17:25

Reformat url

647e234

Add default leaflet style

427b771

Get fontawesome icon from node modules

c478082

crisner changed the title ~~Embed feature in demo page~~ [WIP]Embed feature in demo page Dec 10, 2019

crisner added 2 commits December 11, 2019 18:37

Refactor embed control

08e4061

Add embed feature to other demo files

9388519

Commit package-lock.json

cb26ca0

crisner mentioned this pull request Dec 12, 2019

Outreachy project planning publiclab/plots2#6887

Closed

crisner added 2 commits December 13, 2019 11:11

Resolved merge conflict in AllLayers.js

905bb3e

Prevent adding all layers to the map when embed is set to true

bf31b18

crisner mentioned this pull request Dec 13, 2019

Fix baselayer selection in the map's layer control #300

Merged

5 tasks

sagarpreet-chadha requested changes Dec 14, 2019

View reviewed changes

sagarpreet-chadha closed this Dec 14, 2019

This was referenced Dec 17, 2019

Make domain name as a settable option defaulting to the gh-pages #306

Closed

Display directions on top of map in the demo page #308

Closed

crisner mentioned this pull request Feb 4, 2020

Outreachy Project Recap: summary, planning & discussion publiclab/plots2#7432

Closed

		@@ -121,15 +121,18 @@ L.LayerGroup.environmentalLayers = L.LayerGroup.extend(
		}

		if(this.options.embed) {

[WIP]Embed feature in demo page #296

[WIP]Embed feature in demo page #296

Conversation

crisner commented Dec 9, 2019 • edited Loading

crisner commented Dec 9, 2019

crisner commented Dec 9, 2019

crisner commented Dec 9, 2019

crisner commented Dec 9, 2019

sagarpreet-chadha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisner Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Dec 9, 2019

crisner commented Dec 10, 2019

crisner commented Dec 10, 2019

jywarren commented Dec 10, 2019 via email

crisner commented Dec 10, 2019

jywarren commented Dec 10, 2019 via email

crisner commented Dec 10, 2019

jywarren commented Dec 10, 2019 via email

crisner commented Dec 11, 2019

crisner commented Dec 11, 2019

crisner commented Dec 11, 2019

jywarren commented Dec 11, 2019

sagarpreet-chadha commented Dec 11, 2019

crisner commented Dec 11, 2019

sagarpreet-chadha commented Dec 11, 2019 via email

crisner commented Dec 12, 2019

Screenshot for index.html:

Screenshot for oneLinerCodeExample.html:

Screenshot for unearthing-pvd.html:

crisner commented Dec 12, 2019

crisner commented Dec 13, 2019

sagarpreet-chadha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagarpreet-chadha commented Dec 14, 2019

sagarpreet-chadha commented Dec 15, 2019

crisner commented Dec 9, 2019 •

edited

Loading

crisner Dec 10, 2019 •

edited

Loading