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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated Javascript.json/Modified the body #36

Merged
merged 5 commits into from
Oct 20, 2023

Conversation

Priyanshu-bit
Copy link
Contributor

Updated CreateMap, CreateScene,CreateWebMap, CreateWebScene. Removed assign variables after class Initialization.

@hhkaos hhkaos self-requested a review September 28, 2023 04:14
@hhkaos
Copy link
Member

hhkaos commented Sep 28, 2023

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:

  • prefix conventions: It should include the sufix "Props" calloutLargeProps
  • name conventions. I this case, it is using "Pascal case".
    • I would probably suggest adding one more bullet to the conventions saying: "Names should be descriptive and not overlap with the prefix. Description will extend this short description.". Otherwise we end-up with the Intellisense having things like this:
      Screenshot 2023-09-28 at 07 39 53
    • I would probably use as name "Symbol 3D callout" or "Symbol 3D world-scale callout"

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!

@hhkaos
Copy link
Member

hhkaos commented Sep 28, 2023

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

@hhkaos
Copy link
Member

hhkaos commented Sep 30, 2023

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?

@Priyanshu-bit
Copy link
Contributor Author

Thank you for your patience @hhkaos .I'm working in it.

@hhkaos
Copy link
Member

hhkaos commented Oct 11, 2023

No problem @Priyanshu-bit , that's fine 😄 . Thank you for your support!

Copy link
Member

@hhkaos hhkaos left a 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.

@Priyanshu-bit
Copy link
Contributor Author

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

"Refactor code to utilize 'autocast' for Map class instantiation, enhancing code clarity and reducing manual variable assignments."
@Priyanshu-bit Priyanshu-bit requested a review from hhkaos October 17, 2023 16:42
 Updated the prefix, name conventions, arranged body in alphabetical order
Copy link
Member

@hhkaos hhkaos left a 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 Show resolved Hide resolved
@@ -102,7 +103,7 @@
"prefix": "map"
},

"CreateScene": {
"Create scene": {
"body": [
"new Map({",
"\tbasemap: \"${1:streets}\"",
Copy link
Member

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

Copy link
Contributor Author

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

snippets/javascript.json Outdated Show resolved Hide resolved
snippets/javascript.json Outdated Show resolved Hide resolved
snippets/javascript.json Outdated Show resolved Hide resolved
snippets/javascript.json Outdated Show resolved Hide resolved
snippets/javascript.json Outdated Show resolved Hide resolved
snippets/javascript.json Outdated Show resolved Hide resolved
snippets/javascript.json Outdated Show resolved Hide resolved
snippets/javascript.json Outdated Show resolved Hide resolved
Priyanshu-bit and others added 2 commits October 19, 2023 22:52
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.
@hhkaos hhkaos merged commit 87dd627 into Esri:master Oct 20, 2023
@Priyanshu-bit
Copy link
Contributor Author

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!

@hhkaos
Copy link
Member

hhkaos commented Oct 21, 2023

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! 🙏

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.

2 participants