arcgis-js-vscode-snippets icon indicating copy to clipboard operation
arcgis-js-vscode-snippets copied to clipboard

Snippet "body" conventions

Open hhkaos opened this issue 2 years ago • 7 comments

Hello again!,

Sorry I know this issue is long, but I wanted to share with you some doubts that have arisen while I was creating new snippets and my opinion about them.

As mentioned in the previous issue, I think it would be good if we could try to reach an agreement to define minimum conventions to facilitate the contributions and make them as heterogeneous as possible.

  • Q0) Do we use \t instead of spaces?
  • Q1) Then snippet should containt a new class init, it constructor properties, or both?
  • Q2) When creating new class, do we also assign it to a variable?
  • Q3) Should we avoid adding multiple expressions in one snippet?.
  • Q4) Do we allow comments?
  • Q5) When instantiating a new class, do we add only required fields, the most common ones, all?
  • Q6) When the property expects another class but supporting autocasting, what do we do. Add a sample or place the snippet name?
  • Q7) When a property is overloaded, should we show all possible inputs?
  • Q8) How do we create snippets for inherited methods
  • Q9) Do we allow snippets for enums?

Q0) Do we use \t instead of spaces?

{
  "tsconfig-basic": {
    "prefix": "tsConfigBasic",
    "body": [
      "{",
      "\t\"compilerOptions\": {",
      "\t\t\"module\": \"amd\",",
      "\t\t\"sourceMap\": true,",
      "\t\t\"target\": \"es2019\",",
      "\t\t\"esModuleInterop\": true",
      "\t}",
      "}"
    ],
    "description": "Simple TS Config"
}

or

{
  "tsconfig-basic": {
    "prefix": "tsConfigBasic",
    "body": [
      "{",
      "  \"compilerOptions\": {",
      "    \"lib\": [\"dom\", \"es2015.promise\", \"es5\"],",
      "    \"module\": \"amd\", // output files as AMD modules",
      "    \"sourceMap\": true,",
      "    \"target\": \"es5\",",
      "    \"esModuleInterop\": true",
      "  }",
      "}",
   ],
  "description": "Simple TS Config"
}

A: Yes, I think we should if we want to be more friendly to different tab configurations 😄

Q1) Then snippet should contain a new class initialization, its constructor properties, or both?

Ex: simpleMarkerSymbol snippet:

new SimpleMarkerSymbol({
    color: [255, 255, 255, 0.25],
    size: 12,
    style: "circle",
    outline: {
        width: 1,
        color: [255, 255, 255, 1]
    }
})

or

{
    type: "simple-marker",
    color: [255, 255, 255, 0.25],
    size: 12,
    style: "circle",
    outline: {
        width: 1,
        color: [255, 255, 255, 1]
    }
}

A: In my opinion we could add both (and call them something like simpleMarkerSymbol, simpleMarkerSymbolProps) or simply leave the second one. But in that case, I would add the Class name and the module path in the description.


Q2) When creating a new class, do we also assign it to a variable?

Ex: featureLayer snippet:

new FeatureLayer({
    url: "service-url"
    blendMode "service-url":
    effect: FeatureEffect
});

or

const fl = new FeatureLayer({
    url: "service-url"
    blendMode "service-url":
    effect: FeatureEffect
});

A: I think I would remove the variable. But I think it is a minor thing.


Q3) Should we avoid adding multiple expressions in one snippet?.

Ex: webmap snippet:

const webmap = new WebMap({
    portalItem: {
        id: "webmap_id"
    }
});

// Is this necessary?
const view = new MapView({
    container: "viewDiv",
    map: webmap
});

or featureLayer snippet:

const fl = new FeatureLayer({
    url: "service-url"
    blendMode "service-url":
    effect: FeatureEffect
});

// Is this necessary?
map.add(fl);

or disableNavigation snippet:

view.when(view => {
    const stopEvtPropagation = event => {
        event.stopPropagation();
    }

    // Are all these necessary?
    view.on(["mouse-wheel", "double-click", "drag"], stopEvtPropagation);
    view.on("double-click", ["Control"], stopEvtPropagation);
    view.on("drag", ["Shift"], stopEvtPropagation);
    view.on("drag", ["Shift", "Control"], stopEvtPropagation);
    
    view.on("key-down", function (event) {
        const prohibitedKeys = ["+", "-", "Shift", "_", "=", "ArrowUp", "ArrowDown", "ArrowRight", "ArrowLeft"];
        const keyPressed = event.key;
        if (prohibitedKeys.indexOf(keyPressed) !== -1) {
            event.stopPropagation();
        }
    });
    
    return view;
});

A: I think snippets should be as short as possible. If it is to help init a layer we should only do the init and nothing else. In some cases snippets like disable mapview navigation will be larger.


Q4) Do we allow comments?

Ex: queryLayer snippet:

myLayer  // queries all features in the layer
    .queryFeatures().then(results => {
        // queries features and returns a FeatureSet
    })
    .queryExtent().then(results => {
        // queries features returns extent of features that satisfy query
    })
    .queryFeatureCount().then(results => {
        // queries features and returns count of features
    })
    .queryObjectIds().then(results => {
        // queries features and returns objectIds array of features
    })

A: I think it is OK to add comments


Q5) When instantiating a new class, do we add only required fields, the most common ones, all?

E.x: popupTemplate snippet:

Note: do we use comments to indicate which are required? What do we do when none is required?

{
    outFields: ["*"],
    title: "Value: {attribute}",
    id: "myId",
    className: "esri-icon-zoom-out-magnifying-glass",
    content: [popupContent],
    fieldInfos: [fieldInfo],
    expressionInfos: [expressionInfo],
    actions: [ActionButton],
    returnGeometry: true
}

A: I don't have a clear answer to this. I would probably add the most used ones with a // recommended comment or similar.


Q6) When the property expects another class but supporting autocasting, what do we do?. Add a sample or place the snippet name?

E.x: heatmapRenderer snippet:

new HeatmapRenderer({
    field: "crime_count",
    colorStops: [
        { ratio: 0, color: "rgba(255, 255, 255, 0)" },
        { ratio: 0.2, color: "rgba(255, 255, 255, 1)" },
        { ratio: 0.5, color: "rgba(255, 140, 0, 1)" },
        { ratio: 0.8, color: "rgba(255, 140, 0, 1)" },
        { ratio: 1, color: "rgba(255, 0, 0, 1)" }
    ],
    minPixelIntensity: 0,
    maxPixelIntensity: 5000
})

or

new HeatmapRenderer({
    field: "crime_count",
    colorStops: HeatmapColorStop[],
    minPixelIntensity: 0,
    maxPixelIntensity: 5000
})

A: I prefer the snippet name


Q7) When a property can hold different value types, should we add a placeholder to show all possible inputs?

E.x. PopupTemplate properties:

Name Type
content Content[]|String|Function|Promise
title String|Function|Promise
... ...

Do we do something like this?

{
    "prefix": "popupTemplate",
    "body": [
      "{",
      "\toutFields: [\"*\"],",
      "\ttitle: ${1|\"Value: {attribute}\",function(){}|},",
      "\tid: ${2:\"myId\"},",
      "\tclassName: \"${3:esri-icon-zoom-out-magnifying-glass}\",",
      "\tcontent: ${4|[popupContent],\"<p>Content</p>\",function(){}|},",
      "\tfieldInfos: [fieldInfo],",
      "\texpressionInfos: [expressionInfo],",
      "\tactions: [ActionButton],",
      "\treturnGeometry: ${5|true,false|}",
      "}"
    ],
    "description": "Create an instance of esri/PopupTemplate. Describe popup content"
}

A: I think it is nice to provide all possible input types, knowing that it might not always be up-to-date. Knowing that in case of been using @types/arcgis-js-api it might be kind of duplcated (but it still can add some value like specify the value that needs to be returned by the function, or the sintax to use in the string can referer to geometry fields: 2021-07-19_13-39-19


Q8) How do we create snippets for inherited methods

Ex: method fromJSON() inherited in the SimpleRenderer, UniqueValueRenderer, HeatmapRenderer, etc.

Do we do something like this?:

${1|Simple,UniqueValue,ClassBreaks,Dictionary,DotDensity,Heatmap|}Renderer.fromJSON(json)

A: to make them generic I would try to do so.

Q9) Do we allow snippets for enums?

Ex:

  • rendererTypes: "class-breaks"|"dictionary"|"dot-density"|"heatmap"|"simple"|"unique-value"
  • basemapStyle: arcgis-imagery,arcgis-imagery-standard,arcgis-imagery-labels,arcgis-light-gray,arcgis-dark-gray,arcgis-navigation,arcgis-navigation-night,arcgis-streets,arcgis-streets-night,arcgis-streets-relief,arcgis-topographic,arcgis-oceans,osm-standard,osm-standard-relief,osm-streets,osm-streets-relief,osm-light-gray,osm-dark-gray,arcgis-terrain,arcgis-community,arcgis-charted-territory,arcgis-colored-pencil,arcgis-nova,arcgis-modern-antique,arcgis-midcentury,arcgis-newspaper,arcgis-hillshade-light,arcgis-hillshade-dark
  • symbolTypes: "simple-marker"|"picture-marker"|"simple-line"|"simple-fill"|"picture-fill"|"text"|"shield-label-symbol"|"point-3d"|"line-3d"|"polygon-3d"|"web-style"|"mesh-3d"|"label-3d"|"cim"
  • ...

A: I think I would allow them with the suffix "enum". Something like rendererTypesEnum, basemapsEnum, symbolTypesEnum, ...

hhkaos avatar Jul 19 '21 11:07 hhkaos