Implement new menu in one liner code #353

crisner · 2020-01-17T16:53:34Z

Fixes #347 (<=== 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 · 2020-01-17T17:01:10Z

Things done:

Implemented new menu in one liner code
Add new menu dependancies to one-liner code example files
Fix styles from overriding default leaflet styles

Things to do:

Fix error from layer when refreshing map view when the layer is out of bounds(when using include)
Add option to use default layer control when needed
Add option to display all layers when map is initialized
Add documentation

jywarren · 2020-01-17T18:52:12Z

This is great. Do you mean things to do in a future PR and this one is ready? Or still a work in progress? Thank you!!!

…

On Fri, Jan 17, 2020 at 12:01 PM Renisha Christie. A < ***@***.***> wrote: Things done: - Implemented new menu in one liner code - Add new menu dependancies to one-liner code example files - Fix styles from overriding default leaflet styles Things to do: - Fix error from layer when refreshing map view when the layer is out of bounds(when using include) - Add option to use default layer control when needed - Add option to display all layers when map is initialized - Add documentation — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#353?email_source=notifications&email_token=AAAF6JZZTYF4OOE5FCM2O2LQ6HP5PA5CNFSM4KILFZY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJIKK5I#issuecomment-575710581>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6JYG2LL3ITXQLXOIALTQ6HP5PANCNFSM4KILFZYQ> .

crisner · 2020-01-17T19:07:25Z

This is still a work in progress. Things to do are what's left to do in this PR. I shall make sure to mention when the PR is ready. Thanks!

jywarren · 2020-01-17T20:23:36Z

ok, looks awesome, and keep up the great work!

…

On Fri, Jan 17, 2020 at 2:12 PM Renisha Christie. A < ***@***.***> wrote: This is still a work in progress. Things to do are what's left to do in this PR. I shall make sure to mention when the PR is ready. Thanks! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#353?email_source=notifications&email_token=AAAF6J46BSKDEZI2DCPIACDQ6H6W3A5CNFSM4KILFZY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJIVLFI#issuecomment-575755669>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J62WD4JOUQF2IMPCADQ6H6W3ANCNFSM4KILFZYQ> .

sagarpreet-chadha · 2020-01-18T05:01:57Z

src/AllLayers.js

      var baseMaps = this.options.baseLayers ? this.options.baseLayers : { "Grey-scale": this.options.defaultBaseLayer.addTo(map) };

      for (let layer of this.options.layers.include) {
        if (this.options.layers0.includes(layer)) {
          this.overlayMaps[layer] = window['L']['layerGroup'][layer]();
+          if (layer === 'purpleLayer' && !this.groupedOverlayMaps.PurpleAir) {


Hey this is getting messy, can we use switch case instead?

Sure, will do.

sagarpreet-chadha · 2020-01-18T05:05:26Z

example/styles.css

    overflow-y: hidden;
  }

-  .leaflet-control-layers-overlays {
+  .leaflet-control-layers-menu .leaflet-control-layers-overlays {


We will need this CSS as well in plots2, right?
Also we need to make sure that we use different CSS for different custom bars. Thanks!

We will need this CSS as well in plots2, right?

Thanks for mentioning this. I had assumed it would be bundled into dist. I think it is safe to move the styles to LeafletEnvironmentalLayers.css in /dist. What do you think?

Also we need to make sure that we use different CSS for different custom bars.

Yes, you are right. That is why I had added .leaflet-control-layers-menu which is a custom class so that it does not override leaflet's default .leaflet-control-layers-overlays.

sagarpreet-chadha · 2020-01-18T05:08:13Z

src/AllLayers.js

@@ -129,7 +178,8 @@ L.LayerGroup.environmentalLayers = L.LayerGroup.extend(
        ) : L.control.embed().addTo(map);
      }

-      L.control.layers(baseMaps, this.overlayMaps).addTo(map);


Let's just add if option has param to show default, use this line else use the below layerbrowser line.

Working on this.

crisner · 2020-01-20T08:21:39Z

PR ready for review. The implementation can be checked at https://crisner.github.io/leaflet-environmental-layers/example/oneLinerCodeExample.html

crisner · 2020-01-20T13:23:40Z

README.md

+Dependencies:
+```
+  <!-- Bootstrap --> 
+  <link rel="stylesheet" href="../node_modules\bootstrap\dist\css\bootstrap.min.css">


@sagarpreet-chadha, this just occurred to me. To add these dependencies shouldn't I point it to LEL's node modules? So here, shouldn't I be writing "leaflet-environmental-layers/node_modules" instead to make sense? It wouldn't work otherwise unless the destination repo had bootstrap installed, right?

Found that only LBL was in the node_modules folder in LEL when installing as a package. I was of the impression that all dependencies of a package would be included in the node_modules of that package. So I am assuming we should explicitly mention in the documentation that bootstrap needs to be installed? The new menu needs bootstrap load correctly.

Yes let's do that!

sagarpreet-chadha · 2020-01-21T15:47:42Z

README.md

+Dependencies:
+```
+  <!-- Bootstrap --> 
+  <link rel="stylesheet" href="../node_modules\bootstrap\dist\css\bootstrap.min.css">


Yes let's do that!

sagarpreet-chadha · 2020-01-21T15:51:00Z

example/embed.html

@@ -42,6 +47,11 @@
  <script src="../lib/glify.js"></script>

  <script src="../lib/leaflet-fullUrlHash.js"></script>
+
+  <script src="../node_modules/leaflet-google-places-autocomplete/src/js/leaflet-gplaces-autocomplete.js"></script>


We have documented this also?

I don't think so. This needs to be added too.

Added to readme.md

sagarpreet-chadha · 2020-01-21T15:52:26Z

src/AllLayers.js

@@ -46,7 +47,7 @@ L.LayerGroup.environmentalLayers = L.LayerGroup.extend(
        this.options.baseLayers = param.baseLayers;
      }
      if (!!param.include) {
-        this.options.addLayersToMap = true;
+        this.options.addLayersToMap = param.addLayersToMap === false ? false : true;


Suggested change

this.options.addLayersToMap = param.addLayersToMap === false ? false : true;

this.options.addLayersToMap = param.addLayersToMap;

Done. To display layers by default on inline maps in plots2 we'll need to add addLayersToMap: true to the LEL initialization.

sagarpreet-chadha · 2020-01-21T15:57:13Z

src/util/layersBrowser.js

+      if (isPageRefreshed < 2 && window.performance.navigation.type !== 1) {
+        isPageRefreshed++;  // Track page moveend events to prevent errors on map.removeLayer when a page is reloaded
+      }
+      if(isPageRefreshed > 1) {
+        self._hideElements(obj, data, layerName, elements, true);
+      }


What these lines do?

The new layer menu removes the map on 'moveend' event if the layer is displayed on the map but is out of bounds. The 'moveend' event was being triggered on page refresh causing the error on the unearthing layer where it couldn't find the layer to remove from the map.

This was weird because that line of code would only run if map.hasLayer() returned true. And also I could see the layer displayed on the map and could access it on the console. So I set this up to avoid running that code if it is the first 'moveend' event after a page refresh.

sagarpreet-chadha

I think it is good to go now :) Thanks!

jywarren · 2020-01-23T22:30:22Z

Awesome!!!

crisner added 3 commits January 17, 2020 22:19

Implement new menu

342d821

Add new menu dependancies

1b5dfc8

Fix styles from overriding default leaflet classes

f5be444

crisner changed the title ~~Implement new menu in one liner code~~ [WIP] Implement new menu in one liner code Jan 17, 2020

sagarpreet-chadha requested changes Jan 18, 2020

View reviewed changes

crisner added 10 commits January 18, 2020 15:55

Refactor code

7d5ee46

Fix styles for layer group name and layer description

56175cd

Add option to disable layer visibility when param.include is true

fac5c3a

Add option to use default layer control

fe436ea

Fix errors when trying to display layer when out of bounds

81774fe

Add missing viewport meta tag

f7075b4

Move styles to LeafletEnvironmentalLayers.css

fb6d892

Update documentation

9858317

Add missing dependencies and update build

1706ce1

Fix error from unearthing layer on page reload

1c83345

crisner changed the title ~~[WIP] Implement new menu in one liner code~~ Implement new menu in one liner code Jan 20, 2020

crisner mentioned this pull request Jan 20, 2020

Outreachy project planning publiclab/plots2#6887

Closed

crisner commented Jan 20, 2020

View reviewed changes

sagarpreet-chadha requested changes Jan 21, 2020

View reviewed changes

crisner mentioned this pull request Jan 22, 2020

version bump LEL 2.1.9 publiclab/plots2#7299

Closed

crisner added 2 commits January 22, 2020 15:59

Set addLayersToMap options from param

58560d5

Update documentation

1893e82

sagarpreet-chadha approved these changes Jan 23, 2020

View reviewed changes

sagarpreet-chadha merged commit 7cf1aa1 into publiclab:master Jan 23, 2020

nstjean mentioned this pull request Jan 23, 2020

layersBrowser default layers objects #362

Closed

crisner mentioned this pull request Feb 4, 2020

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new menu in one liner code #353

Implement new menu in one liner code #353

crisner commented Jan 17, 2020 •

edited

Loading

crisner commented Jan 17, 2020

jywarren commented Jan 17, 2020 via email

crisner commented Jan 17, 2020

jywarren commented Jan 17, 2020 via email

sagarpreet-chadha Jan 18, 2020

crisner Jan 18, 2020

sagarpreet-chadha Jan 18, 2020

crisner Jan 18, 2020

crisner Jan 18, 2020

sagarpreet-chadha Jan 18, 2020

crisner Jan 18, 2020

crisner commented Jan 20, 2020

crisner Jan 20, 2020

crisner Jan 21, 2020

sagarpreet-chadha Jan 21, 2020

sagarpreet-chadha Jan 21, 2020

sagarpreet-chadha Jan 21, 2020

crisner Jan 22, 2020

crisner Jan 22, 2020

sagarpreet-chadha Jan 21, 2020

crisner Jan 22, 2020

sagarpreet-chadha Jan 21, 2020

crisner Jan 22, 2020

sagarpreet-chadha Jan 23, 2020

sagarpreet-chadha left a comment

jywarren commented Jan 23, 2020

	this.options.addLayersToMap = param.addLayersToMap === false ? false : true;
	this.options.addLayersToMap = param.addLayersToMap;

Implement new menu in one liner code #353

Implement new menu in one liner code #353

Conversation

crisner commented Jan 17, 2020 • edited Loading

crisner commented Jan 17, 2020

jywarren commented Jan 17, 2020 via email

crisner commented Jan 17, 2020

jywarren commented Jan 17, 2020 via email

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

crisner commented Jan 20, 2020

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

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 left a comment

Choose a reason for hiding this comment

jywarren commented Jan 23, 2020

crisner commented Jan 17, 2020 •

edited

Loading