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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

standardized indigenous layer code #207

Merged
merged 8 commits into from
Jun 27, 2019

Conversation

ananyaarun
Copy link
Member

@ananyaarun ananyaarun commented Jun 13, 2019

Fixes #198

  • 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

Thanks!

@ananyaarun ananyaarun changed the title standardized indigenous layer code [WIP] standardized indigenous layer code Jun 13, 2019
@ananyaarun
Copy link
Member Author

Passing url as parameter in the indigenous layers. Still trying to rectify the CORS error I am getting.
I tried passing the Indigenous layer name as parameter instead and using a if else condition to assign urls respectively but this is still giving the same error.

corserror

@sagarpreet-chadha
Copy link
Collaborator

Hey! The approach looks correct 馃憤
I will take a detailed look later today . Thank you!

@ananyaarun
Copy link
Member Author

No problem @sagarpreet-chadha , I'm still trying ways to remove the CORS error. Thanks!

@sagarpreet-chadha
Copy link
Collaborator

Hey @ananyaarun , how is it going?

}
else{
ILL_url = "https://native-land.ca/api/index.php?maps=treaties&position=" + parseInt(origin.lat) + "," + parseInt(origin.lng);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @sagarpreet-chadha , I tried both passing the ILL_url as a parameter and this if else block like this. But still getting the same cors error. I haven't been able to figure out another way yet. Do you have any other way in mind ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here as we are defining URL again , so lets pass name variable everywhere in arguments,etc. and remove URL variable in function argument . I hope it makes sense .

var IndigenousLandsTerritories = L.layerGroup.indigenousLandsTerritoriesLayer();
var IndigenousLandsLanguages = L.layerGroup.indigenousLandsLanguagesLayer();
var IndigenousLandsTreaties = L.layerGroup.indigenousLandsTreatiesLayer();
var IndigenousLandsTerritories = L.layerGroup.indigenousLayers('https://native-land.ca/api/index.php?maps=territories&position=45,-72',"Territories");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @ananyaarun , why are we passing URL here? I see we are not using it , right? Can we remove this as this will make use of this layer easy ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh,so this I passed as the url parameter. It is being used @sagarpreet-chadha
the section of the code where url is defined in options, it is used there.-
url: this.url,
The 3 indigenous layers have different url's.

var IndigenousLandsTreaties = L.layerGroup.indigenousLandsTreatiesLayer();
var IndigenousLandsTerritories = L.layerGroup.indigenousLayers('https://native-land.ca/api/index.php?maps=territories&position=45,-72',"Territories");
var IndigenousLandsLanguages = L.layerGroup.indigenousLayers('https://native-land.ca/api/index.php?maps=languages&position=45,-72',"Languages");
var IndigenousLandsTreaties = L.layerGroup.indigenousLayers('https://native-land.ca/api/index.php?maps=treaties&position=44,-80',"Treaties");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here 馃槃 .

options: {
url: 'https://native-land.ca/api/index.php?maps=territories&position=45,-72',
url: this.url,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's replace this name: this.name ,

@@ -148,6 +138,7 @@ L.LayerGroup.IndigenousLandsTerritoriesLayer = L.LayerGroup.extend(
}
);

L.layerGroup.indigenousLandsTerritoriesLayer = function (options) {
return new L.LayerGroup.IndigenousLandsTerritoriesLayer(options);
L.layerGroup.indigenousLayers = function (url,name,options) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing URL from here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

In this section of the code i basically pass url and name as a parameter both of which are being used.

L.layerGroup.indigenousLayers = function (url,name,options) {
        
    return new L.LayerGroup.IndigenousLayers(url,name,options);
};

url is the unique url for the 3 layers
name as in territories, languages, treaties


if(this.name == "Territories"){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use this :

Suggested change
if(this.name == "Territories"){
if(this.name === "Territories"){

Copy link
Member Author

Choose a reason for hiding this comment

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

sure :)

@sagarpreet-chadha
Copy link
Collaborator

sagarpreet-chadha commented Jun 17, 2019

I have added some suggestions above .
Regarding CORS error, it should not be there given the fact we are able to use the URL currently .

@jywarren ...What could be the reason?

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 17, 2019

@sagarpreet-chadha , I have not changed the url part yet as it is being used.
Also using an if else condition here would make the code messy and i am not sure if it will work either , as it is just assigning the url in the options (it is not a function)

options: {
            url: this.url,
            popupOnMouseover: false,
            clearOutsideBounds: false,
            target: '_self',
        },

@sagarpreet-chadha
Copy link
Collaborator

I was implying to remove URL field from options , makes sense?
Then we donot need to pass URL in the function, what do you think?

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 17, 2019

@sagarpreet-chadha , I made the required changes. I initially misunderstood what u said.
Btw the cors error is resolved now 馃槂
Looks like now that we are not passing any parameters, there is no problem !!
Here is a working gif :)

trial1

Thanks for the review !!

@sagarpreet-chadha @rexagod let me know what you think.

@ananyaarun ananyaarun requested a review from rexagod June 19, 2019 08:25
var ILL_url;

if(this.name === "Territories"){
ILL_url = "https://native-land.ca/api/index.php?maps=territories&position=" + parseInt(origin.lat) + "," + parseInt(origin.lng);
Copy link
Member

Choose a reason for hiding this comment

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

How about using eval here instead of this if ladder?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, but in that case won't all three conditions be run unnecessarily ? even if the first one is found to be true ?

Copy link
Member

Choose a reason for hiding this comment

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

I've suggested some changes on the particular line of code in discussion above. Take a look!

@@ -38,49 +37,35 @@ L.LayerGroup.IndigenousLandsTerritoriesLayer = L.LayerGroup.extend(
(function() {
var zoom = self._map.getZoom(), origin = self._map.getCenter() ;
var $ = window.jQuery;
var ILL_url;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var ILL_url;
var ILL_url = this.name === "Territories" || this.name === "Languages" ? "https://native-land.ca/api/index.php?maps="+this.name+"&position=" + parseInt(origin.lat) + "," + parseInt(origin.lng) : ILL_url = "https://native-land.ca/api/index.php?maps=treaties&position=" + parseInt(origin.lat) + "," + parseInt(origin.lng);

Okay so since jshint does not take kindly to eval operations, and since we aren't considering doing a //jshint ignore:line here (as that should be the last resort), how about replacing the if ladder with a ternary at the time of declaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this would work!!
Do you feel it makes the code a bit to long for one line and not understandable though ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, linting standards do not recommend any line exceeding more than 80 characters, so I guess you can format this line over a few lines, which should comply with the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll make that change :) Thanks.
But any specific reason why the use of this is better than the use of the if else if block ?

Copy link
Member

Choose a reason for hiding this comment

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

Actually I'd like to emphasize more on analyzing the similarity between these lines abstracting them into a function for a much improved code quality (less code, also reusable). The ternary and spread is something I'd like to go with in such situations, but leave it completely on your acumen to decide to keep them or not.

@@ -82,10 +82,10 @@ L.LayerGroup.IndigenousLayers = L.LayerGroup.extend(
var ill_poly ;
Copy link
Member Author

Choose a reason for hiding this comment

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

@rexagod , how can we possibly use the ternary operator here (in the second if else block)?
All 3 assignments to ill_poly are very different ie not only is the name different (hence this.name based assignment wont work) All three of them are linked to a separate link here.(href part)

Copy link
Member

@rexagod rexagod Jun 19, 2019

Choose a reason for hiding this comment

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

function getPoly(coords, clr, ...stringparams) {
  return L.polygon(coords, {color: clr}).bindPopup(
    `<strong>Name : </strong> ${stringparams.nme} <br>
    <strong>Description: </strong> <a href=${stringparams.desc}>Native Lands - ${stringparams.nme}</a><br>
    <i>From the <a href='https://github.com/publiclab/leaflet-environmental-layers/pull/${
      this.name === "Territories" ? 77 : this.name === "Languages" ? 76 : 78
    }'>Indigenous ${this.name} Inventory</a> (<a href='https://publiclab.org/notes/sagarpreet/06-06-2018/leaflet-environmental-layer-library?_=1528283515'>info<a>)</i>`
  );
}

var ill_poly = (isNaN(coords[0][0][0]) || isNaN(coords[0][0][1])) ? undefined : getPoly(...options);

@ananyaarun Let me know you have any more doubts or so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @rexagod, Thanks for the review!
I tried this code. It initially gave me a reference to option not defined error on my console. Then I removed options passed coords and clr and now it works but the Language layer is loading for both territories and Treties layer as well unlike before with the if else block.

Copy link
Member

Choose a reason for hiding this comment

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

// assuming that `this.name` is defined in this scope

function getPoly(coords, clr, ...stringparams) {
  return L.polygon(coords, {color: clr}).bindPopup(
    `<strong>Name : </strong> ${stringparams[0]} <br>
    <strong>Description: </strong> <a href=${stringparams[1]}>Native Lands - ${stringparams[0]}</a><br>
    <i>From the <a href='https://github.com/publiclab/leaflet-environmental-layers/pull/${
      this.name === "Territories" ? 77 : this.name === "Languages" ? 76 : 78
    }'>Indigenous ${this.name} Inventory</a> (<a href='https://publiclab.org/notes/sagarpreet/06-06-2018/leaflet-environmental-layer-library?_=1528283515'>info<a>)</i>`
  );
}

// options is an array
var options = [coords, 'blue', 'mario', "mario's desc"];

var ill_poly = (isNaN(coords[0][0][0]) || isNaN(coords[0][0][1])) ? undefined : getPoly(...options);

@ananyaarun You need to pass an array containing the required params needed to initialize the getPoly method. The code above was only for reference, so you can copy the logic used, not the code itself, but if you need the exact code, you can use the snippet above (with obvious appropriate changes).

Untitled

PS. I replaced the "L.polygon" and "bindPopup" methods with simple object notations since they weren't defined in the scope, but should work well once you place this inside your js file with all the appropriate scopes defined. Also, one can infer from the image above that the ternary seems to work fine, I hope you get the idea now.

Copy link
Member Author

Choose a reason for hiding this comment

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

function getPoly(coords, clr, ...stringparams) {
  return L.polygon(coords, {color: clr}).bindPopup(
    `<strong>Name : </strong> ${stringparams[0]} <br>
     <strong>Description: </strong> <a href=${stringparams[1]}>Native Lands - ${stringparams[0]}           <a><br><i>From the <a href='https://github.com/publiclab/leaflet-environmental-layers/pull/${
       this.name === "Territories" ? 77 : this.name === "Languages" ? 76 : 78
   }'>Indigenous ${this.name} Inventory</a> (<a href='https://publiclab.org/notes/sagarpreet/06-06-2018/leaflet-environmental-layer-library?_=1528283515'>info<a>)</i>`
  );
}
var array = [coords, clr, 'nme', "desc"];
var ill_poly = (isNaN(coords[0][0][0]) || isNaN(coords[0][0][1])) ? undefined : getPoly(...array);

So here is my console, I see that despite choosing the territories layer, the link has 78 (instead of 77). yes the ternary is working but this.name is not accessible here.
I see that it appears as blank on the link too.

error1

I am trying to see what is going wrong
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@ananyaarun this is actually window in the console, so this.name translates to window.name in the console. The snippet above will work, as I stated, once you place this inside your js file with all the appropriate scopes defined, but if you'd like to test it out in your console, you can this.name="Territorries" (inside the console) or window.name = "Territories" (inside your js file).

@@ -82,10 +82,10 @@ L.LayerGroup.IndigenousLayers = L.LayerGroup.extend(
var ill_poly ;
Copy link
Member

@rexagod rexagod Jun 19, 2019

Choose a reason for hiding this comment

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

function getPoly(coords, clr, ...stringparams) {
  return L.polygon(coords, {color: clr}).bindPopup(
    `<strong>Name : </strong> ${stringparams.nme} <br>
    <strong>Description: </strong> <a href=${stringparams.desc}>Native Lands - ${stringparams.nme}</a><br>
    <i>From the <a href='https://github.com/publiclab/leaflet-environmental-layers/pull/${
      this.name === "Territories" ? 77 : this.name === "Languages" ? 76 : 78
    }'>Indigenous ${this.name} Inventory</a> (<a href='https://publiclab.org/notes/sagarpreet/06-06-2018/leaflet-environmental-layer-library?_=1528283515'>info<a>)</i>`
  );
}

var ill_poly = (isNaN(coords[0][0][0]) || isNaN(coords[0][0][1])) ? undefined : getPoly(...options);

@ananyaarun Let me know you have any more doubts or so.

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 21, 2019

@sagarpreet-chadha @rexagod ,
The initial method I used (if else ) Is incorrect. Same with the ternary condition which follows the same logic as the if else block but has better and less redundant code.
I just realized no matter which layer we choose, the function that is called last (in this case the treaties layer) get loaded. This is what gets called from the html file.

error2

I changed the order in the index.html file. Now the territories layer is called last and this is what my console looks like.

error3

There is no problem with the scope of name I think.

I think passing url would be best,but I/m still not sure about the cors error.

@rexagod
Copy link
Member

rexagod commented Jun 22, 2019

@ananyaarun Can you familiarize me a bit with the context and the specific problem you're facing right now?

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 25, 2019

@rexagod , @sagarpreet-chadha I now got a way to remove the cors error.
I am able to pass the url from the index.html file.
But this still wont work :/
same problem as before.... whichever url is passed last here , that gets loaded.

var IndigenousLandsTerritories = L.layerGroup.indigenousLayers("https://native-land.ca/api/index.php?maps=territories&position=");
    var IndigenousLandsLanguages = L.layerGroup.indigenousLayers("https://native-land.ca/api/index.php?maps=languages&position=");
    var IndigenousLandsTreaties = L.layerGroup.indigenousLayers("https://native-land.ca/api/index.php?maps=treaties&position=");

error4

This is happening because all are accessing the same function.

@rexagod
Copy link
Member

rexagod commented Jun 25, 2019

Pasting this in from gitter:

Hi,
I am responding to the github question here : #207 (comment)
There is an index.html file in example directory from which the JS functions for the layers get called, while defining the layer menu bar.
var IndigenousLandsTreaties = L.layerGroup.indigenousLayers("Treaties");
var IndigenousLandsTerritories = L.layerGroup.indigenousLayers("Territories");
var IndigenousLandsLanguages = L.layerGroup.indigenousLayers("Languages");
Previously these were calling 3 different functions, but now they call the same function after standardization.
In the indigenousLayers function, we have a ternary/if else that is working fine, but since we are using a variable passed to define this,
looks like the function always takes the variable passed last from the function calls in index.html (in this case languages).
Hence no matter which layer is chosen, language layer loads.
Now say we change the order to this
var IndigenousLandsTerritories = L.layerGroup.indigenousLayers("Territories");
var IndigenousLandsLanguages = L.layerGroup.indigenousLayers("Languages");
var IndigenousLandsTreaties = L.layerGroup.indigenousLayers("Treaties");
The treaties layer will load no matter what.
I have been trying a lot of things from the past two days, but nothing seems to work till now.
I hope I have put across my problem properly !! Sorry for the confusion previously, It took me a while to catch this.... I feel that this is the problem.
Thanks

@jywarren
Copy link
Member

Hi @ananyaarun -- i think what may be happening is that each time you use:

L.layerGroup.indigenousLayers();

It is accessing the same object. So any value of "this" is shared among all uses.

What we need is to create a /new/ object each time, so really it seems it has to be a constructor.

This line should really be doing this:

return new L.LayerGroup.IndigenousLayers(url,name,options);

But i think that it may be that the use of this in the LayerGroup definition above, and how the functions are defined as object properties, may be causing us trouble. Because this refers to the class constructor object, not the instance that is produced... if I'm understanding this correctly:

what I often do is, within the constructor function, I just put almost all of the function definitions in there, defined in /only/ local scope, and then I assemble them into an instance object that I return from the constructor function. You can see this pattern here:

https://github.com/publiclab/infragram/blob/main/src/color/colormaps.js

What that does, is it controls the scope of ALL functions to a single shared scope that is only the one where they were defined -- inside the constructor function. And, only those functions which are going to be used externally are passed out in the returned object. All functions can refer to variables defined in the same scope (see this example) and we don't end up using this at all, esp. because this is hard to track. Maybe this is a good architecture to more closely manage your scopes.

Another question i had that maybe @sagarpreet-chadha could offer input on was why this is being defined as a LayerGroup, and not as a Layer, that is then grouped? It seems like perhaps we should be building in functionality to the Layer, not the LayerGroup? https://leafletjs.com/reference-1.5.0.html#layergroup

But I could be wrong, just wondering!

I hope this helps a bit -- also, what is precisely the value that's being overwritten -- is it this.url? If so perhaps that confirms my theory above and is one reason not to use this.

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 26, 2019

Thanks a lot @jywarren !! Name is being overwritten not url.
Yes we need to be careful about the scope of variables here. I'll look through the examples you have linked.

So actually url was initially being passed as one of the options but was not actually being used anywhere (this is not the actual API URL that needs to load)
So I removed this in my latest commits

I was implying to remove URL field from options , makes sense?
Then we donot need to pass URL in the function, what do you think?

Here is the discussion regarding that.

So right now I am just passing the name of the layer (territories,languages, treaties) and there is an if else block to assign the API URL's.

In this method the problem I discussed yesterday with you holds.

As @rexagod suggested we need to find a way to not call the function from index.html while initializing layers. This might work too but I am still confused how this would render multiple layers accessing the same function.

@sagarpreet-chadha
Copy link
Collaborator

Hi @jywarren ,
Yes we could have done by defining extending layer instead of extending layergroup , and grouping markers together and then adding to map .

I think when we started coding LEL it did not made any difference logically but yes now thinking it makes sense we should extend layer . Also the syntax of defining layer would be more easy to use if we switch to extending layer :
L.mapknitter() instead of current L.LayerGroup.mapknitter() .

@sagarpreet-chadha
Copy link
Collaborator

We can extend Layer class via these Minor changes : https://gist.github.com/sagarpreet-chadha/3c864160ed2ea087769e96627f231802 .

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 26, 2019

@sagarpreet-chadha , except for making the syntax of defining the layers easy, will this change have any other effect on the scope of variables ?
also yes this logically makes more sense as you said

Yes we could have done by defining extending layer instead of extending layergroup , and grouping markers together and then adding to map .

@sagarpreet-chadha
Copy link
Collaborator

Hey , i think i have found the problem .
Pasting it on gitter in a moment .

@sagarpreet-chadha
Copy link
Collaborator

Okay so here is the problem :

name1 here refers to a property of parent class which is LayerGroup , so 3 times name1 variable's value is overwiritten and finally having the value of last function call which in our case is "Treaties" . Makes sense ?

  • Currently you may think of name1 as a property of the parent class (similar to how static variable behave in C++) .

Solution :

Change name1 to this.name1 everywhere 馃槃 !

  • because this will always be a unique object for the 3 function calls .

Here is the gist of working code : https://gist.github.com/sagarpreet-chadha/60b4cbfc1cb4fd27d4b1bf600e6d583a

Thank you !

cc @ananyaarun , @jywarren , @rexagod

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 26, 2019

Great catch @sagarpreet-chadha !!
I kept thinking of name1 as a property of the parent class that would keep getting changed as and when they layers are called, but now it totally makes sense why the last call in this case treaties loads all the time.
I'll update this PR and resolve the conflicts.

@sagarpreet-chadha
Copy link
Collaborator

Glad we found it 馃槃 馃帀 !

I think we should write tests for this layer as well , i have setup the Jasmine testing environment here and written some tests as well , see this https://github.com/publiclab/leaflet-environmental-layers/pull/206/files .

@ananyaarun
Copy link
Member Author

ananyaarun commented Jun 26, 2019

Glad we found it smile tada !

I think we should write tests for this layer as well , i have setup the Jasmine testing environment here and written some tests as well , see this https://github.com/publiclab/leaflet-environmental-layers/pull/206/files .

Yes definitely :)
I'll look at it !! Do you think it is better to complete the standardization first and then write tests for all layers or write tests parallely ?

@sagarpreet-chadha
Copy link
Collaborator

I think let's write tests parallely so that we will be more confident to merge PR by any new contributor . Thanks!

@jywarren
Copy link
Member

I'm so glad you figured this out. Will this then be ready for merge now? Awesome teamwork! 馃帀

@ananyaarun
Copy link
Member Author

Yes I think this can be merged, and I shall write tests for this layer in another PR.
@sagarpreet-chadha ?

@sagarpreet-chadha
Copy link
Collaborator

Okay great +1

@sagarpreet-chadha sagarpreet-chadha merged commit 9c8b24c into publiclab:master Jun 27, 2019
@ananyaarun ananyaarun changed the title [WIP] standardized indigenous layer code standardized indigenous layer code Jun 27, 2019
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.

Standardize Layer code in the LEL repository
4 participants