[HERC-178] OSS Comment Cleanup

src/chart-addons/stacked-legend.js

@@ -260,7 +260,7 @@ export async function getLegendStateFromChart(chart, useMap, selectedLayer) {
             version: 2.0
           }
         } else {
-          // TODO(croot): this can be removed once all raster layer types are
+          // TODO: this can be removed once all raster layer types are
           // transitioned to the getPrimaryColorScaleName/getLegendDefinitionForProperty
           // form above
           const layerState = layer.getState()
@@ -845,7 +845,7 @@ function legendState_v2(state, useMap) {
       return {}
     }
     const min_size = Math.min(domain.length, range.length)
-    // TODO(croot): may want to consider filling out the auto-gradient here
+    // TODO: may want to consider filling out the auto-gradient here
     // using a max-number of stops. The best way to do this will most likely
     // be to create a d3 scale of similar type, and generate new ranges from
     // it at auto-gradient stops. Unforunately the legendables library will only

src/charts/geo-choropleth-chart.js

@@ -197,7 +197,7 @@ export default function geoChoroplethChart(parent, useMap, chartGroup, mapbox) {
 
   /* OVERRIDE EXTEND ----------------------------------------------------------*/
   function accentPoly(label) {
-    const layerNameClass = geoJson(0).name // hack for now as we only allow one layer currently
+    const layerNameClass = geoJson(0).name // Currently limited to single layer support
     _chart.selectAll("g." + layerNameClass).each(function(d) {
       if (getKey(0, d) == label) {
         _chart.accentSelected(this)
@@ -206,7 +206,7 @@ export default function geoChoroplethChart(parent, useMap, chartGroup, mapbox) {
   }
 
   function unAccentPoly(label) {
-    const layerNameClass = geoJson(0).name // hack for now as we only allow one layer currently
+    const layerNameClass = geoJson(0).name // Currently limited to single layer support
     _chart.selectAll("g." + layerNameClass).each(function(d) {
       if (getKey(0, d) == label) {
         _chart.unAccentSelected(this)

src/charts/heatmap.js

@@ -47,7 +47,6 @@ export function heatMapKeyAccessor({ key0 }) {
   }
 }
 
-// TODO - remove me. Trying to get TravisCI to work
 export const heatMapKeyAccessorNoFormat = function({ key0 }, forceMax = false) {
   if (Array.isArray(key0)) {
     const keyIndex = forceMax && key0[1] ? 1 : 0

src/charts/raster-chart.js

@@ -235,7 +235,7 @@ export default function rasterChart(parent, useMap, chartGroup, _mapboxgl) {
     }
 
     if (
-      // pointmap prioritized color hack
+      // Special handling for pointmap prioritized color rendering
       layer.getState().mark === "point" &&
       layerName !== "backendScatter" &&
       layer.getState().encoding.color.prioritizedColor &&
@@ -379,7 +379,7 @@ export default function rasterChart(parent, useMap, chartGroup, _mapboxgl) {
     return _chart
   }
 
-  // TODO(croot): pixel ratio should probably be configured on the backend
+  // TODO: pixel ratio should probably be configured on the backend
   // rather than here to deal with scenarios where data is used directly
   // in pixel-space.
   _chart.usePixelRatio = function(usePixelRatio) {
@@ -568,7 +568,7 @@ export default function rasterChart(parent, useMap, chartGroup, _mapboxgl) {
     }
 
     // if _chart.useLonLat() is not true, the chart bounds have already been projected into mercator space
-    // TODO(adb): could probably collape this into line 353
+    // TODO: could probably collape this into line 353
     if (
       _useGeoTypes &&
       typeof _chart.useLonLat === "function" &&
@@ -800,7 +800,7 @@ export default function rasterChart(parent, useMap, chartGroup, _mapboxgl) {
       const vega_metadata = JSON.parse(data.vega_metadata)
       for (const layerName in _layerNames) {
         if (typeof _layerNames[layerName]._updateFromMetadata === "function") {
-          // TODO(croot): I don't understand this logic. Why would the selectedLayer
+          // TODO: I don't understand this logic. Why would the selectedLayer
           // be set only on the existence of the _updateFromMetadata() method?
           // My guess is that it's an attempt to get the last layer, but there are
           // so many better ways to get at that.
@@ -931,7 +931,7 @@ export default function rasterChart(parent, useMap, chartGroup, _mapboxgl) {
 
   _chart.measureValue = function(value, key) {
     const customFormatter = _chart.valueFormatter()
-    // hack to undo the popup concatenation like "AVG(arrdelay)"
+    // Parse aggregation function from popup key format (e.g., "AVG(arrdelay)")
     let keyTrimmed = null
     if (key) {
       keyTrimmed = key.replace(/.*\((.*)\).*/, "$1")
@@ -981,7 +981,7 @@ export default function rasterChart(parent, useMap, chartGroup, _mapboxgl) {
         const layerName = _layers[i]
         const layer = _layerNames[layerName]
         if (layer) {
-          // TODO(croot): can this be improved? I presume only
+          // TODO: can this be improved? I presume only
           // one popup can be shown at a time
           if (animate) {
             layer.hidePopup(_chart, () => {
@@ -1078,7 +1078,7 @@ function checkMultiYScaleLayers(chart, scales) {
     mesh2dLayer.length > 0
   ) {
     // ensure the domain of the y scale is the range set from the mesh2d yDim
-    // TODO(C): need to check how multi 2-D CS's are handled to tell if this is the
+    // TODO: need to check how multi 2-D CS's are handled to tell if this is the
     // correct approach for setting the y domain or not
     scales[1].domain = mesh2dLayer[0].yDim().getFilter()[0]
     // add a new y scale for the terrain layer, updating the name in the layer and setting
@@ -1128,7 +1128,7 @@ function genLayeredVega(chart) {
       nullValue: -100
     }
   ]
-  // TODO(matzy): This can probably be cleaned up now that chart.y2() exists
+  // TODO: This can probably be cleaned up now that chart.y2() exists
   checkMultiYScaleLayers(chart, scales)
 
   // NOTE(adb): When geo types are enabled, vega spatial projections are applied and the scales for the x and y properties are not being used. However, we still need the legacy scaling terms to properly size poly popups on hover, which is why _xLatLngBnds, etc are separate scales

src/charts/row-chart.js

@@ -134,7 +134,7 @@ export default function rowChart(parent, chartGroup) {
     const key = _chart.getMeasureName()
     // We have no good way of knowing if the valueFormatter has a formatter for the X axis in
     // particular, since that code is a black box. So we run through a test value and if it returns
-    // `null`, we know it doesn't know how to format it. It's dumb, but it works.
+    // `null`, we know it doesn't know how to format it.
     const validFormatting = numberFormatter && numberFormatter(0, key) !== null
     if (validFormatting) {
       xFormatCache.setTickFormat(d => numberFormatter(d, key))

src/core/core.js

@@ -226,7 +226,7 @@ export function transition(selections, duration, callback, name) {
   return s
 }
 
-/* somewhat silly, but to avoid duplicating logic */
+/* to avoid duplicating logic */
 export function optionalTransition(enable, duration, callback, name) {
   if (enable) {
     return function(selection) {

src/mixins/coordinate-grid-raster-mixin.js

@@ -202,7 +202,7 @@ export default function coordinateGridRasterMixin (_chart, _mapboxgl, browser) {
       return _maxBounds
     }
 
-    // TODO(croot): verify max bounds?
+    // TODO: verify max bounds?
     if (!(maxBounds instanceof Array) || maxBounds.length !== 2 || !(maxBounds[0] instanceof Array) || maxBounds[0].length !== 2 || !(maxBounds[1] instanceof Array) || maxBounds[1].length !== 2) {
       throw new Error("Invalid bounds argument. A bounds object should be: [[xmin, ymin], [xmax, ymax]]")
     }
@@ -707,7 +707,7 @@ export default function coordinateGridRasterMixin (_chart, _mapboxgl, browser) {
 
 
     if (prevWidth !== _chartBody.style("width") || prevHeight !== _chartBody.style("height")) {
-      // TODO(croot): What about when the margins change?
+      // TODO: What about when the margins change?
       // That's not truly a resize event
       _chart.map().fire("resize", {
         width,
@@ -879,7 +879,7 @@ export default function coordinateGridRasterMixin (_chart, _mapboxgl, browser) {
     }
     _lastXDomain = xdom
 
-    // TODO(croot): support ordinal scales?
+    // TODO: support ordinal scales?
     // If BE supports ordinal scales for X axis, use
     // rangeBands here: i.e. x.rangeBands([0, _chart.xAxisLength()], ...)
 
@@ -1077,7 +1077,7 @@ export default function coordinateGridRasterMixin (_chart, _mapboxgl, browser) {
     }
 
     if (text !== "") {
-      // TODO(croot): should add the rotation and labelXPosition here
+      // TODO: should add the rotation and labelXPosition here
       // As of now (09/02/2016) the chart.css is breaking this.
 
       if (axisClass === "y2") {

src/mixins/dc-legend-mixin.js

@@ -72,7 +72,7 @@ export default function legendMixin(legend) {
 
       const bodyNode = body.node()
       if (bodyNode) {
-        // fix for #4196#issuecomment-376704328
+        // Preserve scroll position when legend is redrawn (dc.js issue #4196)
         bodyNode.scrollTop = legend._scrollPos
       }
     }

src/mixins/lock-axis-mixin.js

@@ -264,8 +264,8 @@ export default function lockAxisMixin(chart) {
       .domain()
       .slice()
 
-    // Horrible hack to ensure the inputs aren't inverted from whatever order
-    //  the Y axis decides to display.  Mea culpa.
+    //  Ensure the inputs aren't inverted from whatever order
+    //  the Y axis decides to display.
     let shouldFlipYMinMax = false
     const isHeatY = chart.isHeatMap && type === "y"
     if (isHeatY) {

src/mixins/map-mixin.js

@@ -696,7 +696,7 @@ export default function mapMixin(
           coordinates: boundsToUse
         })
       } else {
-        // for some reason, the source is lost some of the time, so adding it again FE-9833
+        // for some reason, the source is lost some of the time, so adding it again
         setSourceAndAddLayer(overlayName)
       }
     }
@@ -826,7 +826,6 @@ export default function mapMixin(
       _lastWidth = width
       _lastHeight = height
 
-      // this is a dumb hack, but it works and there doesn't seem to be another sensible way.
       // problem is, mapbox looks to the size of "mapboxgl-canvas-container" to determine the
       // render size of the canvas, regardless of what we feed it above. if there is an overlay
       // drawer open, even if it sits at a higher z-index, the canvas size will be calculated

src/mixins/raster-draw-mixin.js

@@ -514,7 +514,7 @@ export function rasterDrawMixin(chart) {
 
   chart.addDrawControl = (lassoToolSetTypes = LassoToolSetTypes.kStandard) => {
     if (drawEngine) {
-      // TODO(croot): if the requested tool set types are different,
+      // TODO: if the requested tool set types are different,
       // should we update the button group controller here?
       return chart
     }

src/mixins/raster-layer-cross-section-terrain-mixin.js

@@ -281,29 +281,29 @@ export default function rasterLayerCrossSectionTerrainMixin(_layer) {
   _layer._addRenderAttrsToPopupColumnSet = function(chart, popupColumnsSet) {
     // currently no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
   }
 
   // eslint-disable-next-line no-unused-vars
   _layer._areResultsValidForPopup = function(results) {
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
     return true
   }
 
   // eslint-disable-next-line no-unused-vars
   _layer._displayPopup = function(svgProps) {
     // currently a no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
     return AABox2d.create()
   }
 
   // eslint-disable-next-line no-unused-vars
   _layer._hidePopup = function(chart, hideCallback) {
     // currently a no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
   }
 
   _layer._destroyLayer = function() {

src/mixins/raster-layer-mesh2d-mixin.js

@@ -20,7 +20,7 @@ import VegaPropertyOutputState from "./render-vega-lite/VegaPropertyOutputState"
 import { LEGEND_POSITIONS } from "../chart-addons/stacked-legend"
 
 // eslint-disable-next-line no-warning-comments
-// TODO(croot): this seems like it is used in at least one other layer mixin. Make into a utility somewhere
+// TODO: this seems like it is used in at least one other layer mixin. Make into a utility somewhere
 function create_post_filter_transform(post_filters) {
   const post_filter =
     post_filters && Array.isArray(post_filters) ? post_filters[0] : null // may change to map when we have more than one postFilter
@@ -260,29 +260,29 @@ export default function rasterLayerMesh2dMixin(_layer) {
   _layer._addRenderAttrsToPopupColumnSet = function(chart, popupColumnsSet) {
     // currently no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
   }
 
   // eslint-disable-next-line no-unused-vars
   _layer._areResultsValidForPopup = function(results) {
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
     return true
   }
 
   // eslint-disable-next-line no-unused-vars
   _layer._displayPopup = function(svgProps) {
     // currently a no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
     return AABox2d.create()
   }
 
   // eslint-disable-next-line no-unused-vars
   _layer._hidePopup = function(chart, hideCallback) {
     // currently a no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support mesh2d hit-testing
+    // TODO: needs to be filled in to support mesh2d hit-testing
   }
 
   _layer._destroyLayer = function() {

src/mixins/raster-layer-point-mixin.js

@@ -239,7 +239,6 @@ export default function rasterLayerPointMixin(_layer) {
         as: alias,
         // For some reason, we're receiving duplicate tables here, causing headaches w/ export SQL generation
         //  in heavyai-data-layer2. So, just gonna filter them out.
-        //  https://heavyai.atlassian.net/browse/FE-14213
         groupby: [...new Set(transform.groupby)].map((g, i) => ({
           type: "project",
           expr: `${isDataExport && i === 0 ? "/*+ cpu_mode */ " : ""}${g}`,
@@ -701,7 +700,7 @@ export default function rasterLayerPointMixin(_layer) {
   }
 
   _layer._genVega = function(chart, layerName, group, query) {
-    // Pointmap prioritized color hack. Need to use the real layer name for crossfilter
+    // Pointmap prioritized color handling: use the actual layer name for crossfilter operations
     let realLayerName = layerName
     if (
       layerName &&

src/mixins/raster-layer-poly-mixin.js

@@ -824,7 +824,7 @@ export default function rasterLayerPolyMixin(_layer) {
         (i !== _layer.dimension().getDimensionIndex() &&
           f !== "" &&
           f !== null &&
-          // Kinda hacky, we don't want to include rowId filter in vega query, but we want everything else
+          // Exclude rowId filter from vega query while including all other filters
           !f.includes("rowid"))
     )
 
@@ -966,7 +966,7 @@ export default function rasterLayerPolyMixin(_layer) {
     return __displayPopup({ ...svgProps, _vega, _layer, state })
   }
 
-  // We disabled polygon selection filter from Master layer if the chart has more than one poly layer in 4.7 release, FE-8685.
+  // We disabled polygon selection filter from Master layer if the chart has more than one poly layer in 4.7 release.
   // Since we run rowid filter on poly selection filter, it is not correct to run same rowid filter for all overlapping poly layers.
   // We need better UI/UX design for this
   function chartHasMoreThanOnePolyLayers(chart) {
@@ -985,7 +985,7 @@ export default function rasterLayerPolyMixin(_layer) {
         chartHasMoreThanOnePolyLayers(chart)) ||
       !_onClickFiltering
     ) {
-      // don't filter from Master, FE-8685
+      // don't filter from Master
       return
     }
     const isInverseFilter = Boolean(event && (event.metaKey || event.ctrlKey))

src/mixins/raster-layer-windbarb-mixin.js

@@ -114,7 +114,6 @@ export default function rasterLayerWindBarbMixin(_layer) {
         as: alias,
         // For some reason, we're receiving duplicate tables here, causing headaches w/ export SQL generation
         //  in heavyai-data-layer2. So, just gonna filter them out.
-        //  https://heavyai.atlassian.net/browse/FE-14213
         groupby: [...new Set(transform.groupby)].map((g, i) => ({
           type: "project",
           expr: `${isDataExport && i === 0 ? "/*+ cpu_mode */ " : ""}${g}`,
@@ -213,7 +212,7 @@ export default function rasterLayerWindBarbMixin(_layer) {
   prop_descriptors.set("y", new PositionChannelDescriptor("y"))
 
   // eslint-disable-next-line no-warning-comments
-  // TODO(croot): support geo columns
+  // TODO: support geo columns
   // prop_descriptors.set(
   //   "longitude",
   //   new GeographicChannelDescriptor("longitude", "x")
@@ -378,26 +377,26 @@ export default function rasterLayerWindBarbMixin(_layer) {
   _layer._addRenderAttrsToPopupColumnSet = function(chart, popupColumnsSet) {
     // currently no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support windbarb hit-testing
+    // TODO: needs to be filled in to support windbarb hit-testing
   }
 
   _layer._areResultsValidForPopup = function(results) {
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support windbarb hit-testing
+    // TODO: needs to be filled in to support windbarb hit-testing
     return true
   }
 
   _layer._displayPopup = function(svgProps) {
     // currently a no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support windbarb hit-testing
+    // TODO: needs to be filled in to support windbarb hit-testing
     return AABox2d.create()
   }
 
   _layer._hidePopup = function(chart, hideCallback) {
     // currently a no-op
     // eslint-disable-next-line no-warning-comments
-    // TODO(croot): needs to be filled in to support windbarb hit-testing
+    // TODO: needs to be filled in to support windbarb hit-testing
   }
 
   _layer._destroyLayer = function() {