feat: Add snippets for all Layer classes [#34] #63
Conversation
There was a problem hiding this comment.
Hi @BochaberiDaisy ! Thank you very much for the PR. It is a great start and a lot of work, I really appreciate it. I have started checking and I have just noticed we need to review to meet the conventions. A few example I noticed so far:
- Prefixes in class constructors should match the Class Capitalization (e.g.
graphicsLayershould be GraphicsLayer;addGeoRSSLayershould beGeoRSSLayer`, etc.). Prefixes starting with lowercase are used for class properties objects, common procedures, and enumerations. If you want to suggest a change, please check the issue Snippet "prefix" convetions #10. - In the body I have seen inconsistencies with map.add(layerVariable) and not storing the variable. I don't have an strong opinion about having the statement or not, but if we add it, the layer variable should be instantiated within the snippet. If you want to suggest a change, please check the issue Snippet "body" conventions #19.
- The descriptions are not following the conventions. Some descriptions are a little bit short. You can reuse text from the API reference. And the class initiatilization do not finish with the paths.
I have added some comments just to illustrate some examples.
Thanks again!
snippets/javascript.json
Outdated
| "CSVLayer({", | ||
| "\turl: \"${1:https://earthquake.usgs.gov/earthquakes/feed/v1.0/summary/2.5_week.csv}\",", | ||
| "\tcopyright: \"${2:USGS Earthquakes}\",", | ||
| "\tpopupTemplate: ${3:template}", |
There was a problem hiding this comment.
| "\tpopupTemplate: ${3:template}", |
The popupTemplate is not mandatory and templateis not a snippet. I think it would be better just to remove it, don't you think?.
There was a problem hiding this comment.
I totally agree
snippets/javascript.json
Outdated
| "\tcopyright: \"${2:USGS Earthquakes}\",", | ||
| "\tpopupTemplate: ${3:template}", | ||
| "});", | ||
| "map.add(csvLayer);" |
There was a problem hiding this comment.
| "map.add(csvLayer);" |
I would suggest keeping it small, especially if csvLayer variable is not even instantiated.
snippets/javascript.json
Outdated
| "\t}", | ||
| "});", | ||
| "", | ||
| "table.load().then(function() {", |
There was a problem hiding this comment.
I feel this table load could be omited
snippets/javascript.json
Outdated
| "\t\t\ttype: \"${6:string}\"", | ||
| "\t\t},", | ||
| "\t\t{", | ||
| "\t\t\tname: \"${7:place}\",", |
There was a problem hiding this comment.
Probably we can remove this place field. It is redundant considering we have other "string field" already..
|
Here is the task list for the progress on the changes requested.
|
There was a problem hiding this comment.
Amazing work @BochaberiDaisy ! 👏 😍
I just added a few minor things. And I would say just make sure to prettify (fix all indentations).
snippets/javascript.json
Outdated
| "});", | ||
| "let url = URL.createObjectURL(blob);", | ||
| "", | ||
| new CSVLayer({", |
There was a problem hiding this comment.
| new CSVLayer({", | |
| "new CSVLayer({", |
There was a problem hiding this comment.
Amazing work @BochaberiDaisy ! 👏 😍
I just added a few minor things. And I would say just make sure to prettify (fix all indentations).
|
@hhkaos, I have pushed the requested changes. Please check it out. Thank you for the feedback! |
There was a problem hiding this comment.
Awesome work @BochaberiDaisy , it looks good to me, thank you very much for the hard work!
Fix: #34
@hhkaos, please check the PR.
I have added snippets to all the Layers.
Let me know if any changes are needed 🙂