Closed Priyanshu-bit closed 10 months ago
Thanks @Priyanshu-bit, I just looked at it and the changes are good. But as you described, this PR includes one commit to apply only one body convention -> After a class initialization, don't assign it to a variable.
I'm missing fixes related to other conventions, for example:
"CalloutLarge": {
"body": [
"verticalOffset: {",
"\tscreenLength: 40,",
"\tmaxWorldLength: 500000,",
"\tminWorldLength: 0",
"},",
"callout: {",
"\ttype: \"line\",",
"\tsize: ${1:1.5},",
"\tcolor: ${2:[255, 255, 255, 0.9]},",
"\tborder: {",
"\t\tcolor: ${3:[0, 0, 0, 0.5]}",
"\t}",
"}"
],
"description": "Generate 3D callouts for world scale",
"prefix": "calloutLarge"
},
In this case:
calloutLargeProps
prefix
. Description will extend this short description.". Otherwise we end-up with the Intellisense having things like this:And body is not in alphabetical order.
verticalOffset: {
screenLength: 40,
maxWorldLength: 500000,
minWorldLength: 0
},
callout: {
type: "line",
size: 1.5,
color: [255, 255, 255, 0.9],
border: {
color: [0, 0, 0, 0.5]
}
}
Use alphabetical order
callout: {
type: "line",
size: 1.5,
color: [255, 255, 255, 0.9],
border: {
color: [0, 0, 0, 0.5]
}
}
verticalOffset: {
screenLength: 40,
maxWorldLength: 500000,
minWorldLength: 0
},
But I noticed when didn't specify if nested objects should also be in alphabetical order. I guess we should comment in https://github.com/Esri/arcgis-js-vscode-snippets/issues/19
I hope this helps.
I'll wait for upcoming commits before merging this. Thanks again for your help!
I forgot to mention. I have also made some changes to the contributing guidelines to help you do better commit&pr messages.
Please check: https://github.com/Esri/arcgis-js-vscode-snippets/pull/38/files
One additional comment, I think the "map" snippet, should be refactored to use autocast, I mean:
new MapView({
container: "viewDiv",
map: {
basemap: "streets"
},
zoom: 4,
center: [15,65]
});
Otherwise, we will be forcing the developer to assign the Map class instance to a variable manually to be able to use it. Thoughts?
Thank you for your patience @hhkaos .I'm working in it.
No problem @Priyanshu-bit , that's fine 😄 . Thank you for your support!
As I mentioned in the comments, these changes look pretty good, but
CreateMap
needs to be refactored in order to work. Sure, will be done by the end of the day
I want to express my sincere thanks to @hhkaos for the invaluable support and guidance you've provided throughout my journey. I'm absolutely thrilled about my recent contribution and eagerly look forward to further collaborations.
Your assistance has been instrumental, and I appreciate it immensely. Here's to more exciting contributions in the future!
I've also learned a lot during this process, so thank you very much @Priyanshu-bit. And thank you for all of your work and effort in this contribution; it is undeniably an essential contribution to the new path of this extension. I look forward to continuing to collaborate with you.
Thanks! 🙏
Updated CreateMap, CreateScene,CreateWebMap, CreateWebScene. Removed assign variables after class Initialization.