-
Notifications
You must be signed in to change notification settings - Fork 15
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated Javascript.json/Modified the body #36
Updated Javascript.json/Modified the body #36
Conversation
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:
In this case:
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 #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:
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the comments, these changes look pretty good, but CreateMap
needs to be refactored in order to work.
|
"Refactor code to utilize 'autocast' for Map class instantiation, enhancing code clarity and reducing manual variable assignments."
Updated the prefix, name conventions, arranged body in alphabetical order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @Priyanshu-bit !
I just noticed you missed a some conventions like the "Props" suffix, or add the AMD & ES paths in the description when the script is initializing a class.
Also check the names
, same have spaces at the start/end, e.g:
- "Add a layer from a portal item " -> "Add a layer from a portal item"
- " Generate water symbol 3D layer" -> "Generate water symbol 3D layer"
- ...
Thanks again, this one is getting close to the goal! :)
snippets/javascript.json
Outdated
@@ -102,7 +103,7 @@ | |||
"prefix": "map" | |||
}, | |||
|
|||
"CreateScene": { | |||
"Create scene": { | |||
"body": [ | |||
"new Map({", | |||
"\tbasemap: \"${1:streets}\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we have a snippet basemapStyle
. I think it would make sense to place it, same with "Create map". What to do think? I will be easier to maintain.
We also have the problem with autocasting below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autocasting done, rest cannot understand properly
Improved code snippet by reformatting the code to enhance indentation, eliminating unnecessary spaces, updating the snippet's name and prefix, used autocasting where needed.
Hi @Priyanshu-bit!, you did a great job. I just wanted to make some additional changes to go over the convention's again before merging. I also took the opportunity to make some additional changes.
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.