feat: remove d3 format option from PopUps

Merge pull request #68 from CartoDB/feature/remove-d3-format
CartoDB · Jul 9, 2020 · d429512 · d429512 · vercel · Jul 9, 2020
2 parents a0fb5e5 + 269af53
commit d429512
Show file tree

Hide file tree

Showing 10 changed files with 26 additions and 65 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,7 +9,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 - Add `beforeLayerId` and `afterLayerId` options to `carto.viz.Layer.addTo` method, to customize layer position
 
 ### Changed
-- 
+- Remove d3.format option from `carto.viz.Popup`
 
 ### Fixed
 - 

diff --git a/examples/_debug/widgets/geojson-formula-data-view-all-modes.html b/examples/_debug/widgets/geojson-formula-data-view-all-modes.html
@@ -142,10 +142,10 @@
     <script src="https://api.tiles.mapbox.com/mapbox-gl-js/v1.10.0/mapbox-gl.js"></script>
     <script src="https://unpkg.com/deck.gl@8.2.0/dist.min.js"></script>
     <script src="https://libs.cartocdn.com/airship-components/v2.3/airship.js"></script>
+    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
     <script src="/dist/umd/index.min.js"></script>
 
-    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
     <script>
       const operationElem = document.querySelector('as-dropdown');

diff --git a/examples/dataview/formula-data-view-all-modes.html b/examples/dataview/formula-data-view-all-modes.html
@@ -142,10 +142,10 @@
     <script src="https://api.tiles.mapbox.com/mapbox-gl-js/v1.10.0/mapbox-gl.js"></script>
     <script src="https://unpkg.com/deck.gl@8.2.0/dist.min.js"></script>
     <script src="https://libs.cartocdn.com/airship-components/v2.3/airship.js"></script>
+    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
     <script src="/dist/umd/index.min.js"></script>
 
-    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
     <script>
       const operationElem = document.querySelector('as-dropdown');

diff --git a/examples/dataview/formula-data-view.html b/examples/dataview/formula-data-view.html
@@ -65,10 +65,10 @@
     <script src="https://api.tiles.mapbox.com/mapbox-gl-js/v1.10.0/mapbox-gl.js"></script>
     <script src="https://unpkg.com/deck.gl@8.2.0/dist.min.js"></script>
     <script src="https://libs.cartocdn.com/airship-components/v2.3/airship.js"></script>
+    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
     <script src="/dist/umd/index.min.js"></script>
 
-    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
     <script>
       // const widgetElement = document.querySelector('as-formula-widget');

diff --git a/examples/interactivity/add-popups-onhover.html b/examples/interactivity/add-popups-onhover.html
@@ -51,13 +51,11 @@ <h1 class="as-title">Pop-ups on hover</h1>
       </div>
     </div>
 
-    <!-- Map libraries-->
     <script src="https://api.tiles.mapbox.com/mapbox-gl-js/v1.10.0/mapbox-gl.js"></script>
     <script src="https://unpkg.com/deck.gl@8.2.0/dist.min.js"></script>
-    <!-- Airship -->
     <script src="https://libs.cartocdn.com/airship-components/v2.3/airship.js"></script>
+    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
-    <!-- CARTO Deck.gl's Tile Layer -->
     <script src="/dist/umd/index.min.js"></script>
 
     <script>
@@ -85,7 +83,7 @@ <h1 class="as-title">Pop-ups on hover</h1>
           {
             attr: 'pop_2015',
             title: 'Population',
-            format: '~s' // D3 format. See https://github.com/d3/d3-format for examples
+            format: d3.format('~s') // D3 format. See https://github.com/d3/d3-format for examples
           }
         ]);
         await populationLayer.addTo(deckMap);

diff --git a/examples/interactivity/popups-format.html b/examples/interactivity/popups-format.html
@@ -45,8 +45,8 @@ <h1 class="as-title">Pop-ups on hover with format</h1>
                       Configure pop-up windows on hover with format.
                     </p>
                     <p class="as-body">
-                      The attribute format accepts a string as D3 format (see
-                      https://github.com/d3/d3-format) or a function as custom format.
+                      The attribute format accepts a function, so for example you can use a D3 format (see
+                      https://github.com/d3/d3-format) or any other arbitrary function.
                     </p>
                   </section>
                 </div>
@@ -57,13 +57,11 @@ <h1 class="as-title">Pop-ups on hover with format</h1>
       </div>
     </div>
 
-    <!-- Map libraries-->
     <script src="https://api.tiles.mapbox.com/mapbox-gl-js/v1.10.0/mapbox-gl.js"></script>
     <script src="https://unpkg.com/deck.gl@8.2.0/dist.min.js"></script>
-    <!-- Airship -->
     <script src="https://libs.cartocdn.com/airship-components/v2.3/airship.js"></script>
+    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3-format/1.3.0/d3-format.min.js"></script>
 
-    <!-- CARTO Deck.gl's Tile Layer -->
     <script src="/dist/umd/index.min.js"></script>
 
     <script>
@@ -94,7 +92,7 @@ <h1 class="as-title">Pop-ups on hover with format</h1>
           {
             attr: 'pop_2015',
             title: 'Population D3',
-            format: '~s' // D3 format see https://github.com/d3/d3-format
+            format: d3.format('~s') // D3 format see https://github.com/d3/d3-format
           },
           {
             attr: 'pop_2015',

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -50,7 +50,6 @@
     "@salte-auth/popup": "1.0.0-rc.2",
     "@salte-auth/salte-auth": "3.0.0-rc.8",
     "@types/chroma-js": "^2.0.0",
-    "@types/d3-format": "^1.3.1",
     "@types/geojson": "^7946.0.7",
     "@types/googlemaps": "^3.39.3",
     "@types/jest": "^24.0.18",
@@ -103,7 +102,6 @@
     "@math.gl/core": "3.2.1",
     "cartocolor": "^4.0.2",
     "chroma-js": "^2.1.0",
-    "d3-format": "^1.4.4",
     "deepmerge": "^4.2.2",
     "live-server": "^1.2.1",
     "mitt": "^1.2.0",

diff --git a/src/lib/viz/__tests__/interactivity-popups.spec.ts b/src/lib/viz/__tests__/interactivity-popups.spec.ts
@@ -51,8 +51,7 @@ describe('interaction popup', () => {
     },
     {
       attr: 'pop',
-      title: 'Population D3',
-      format: '~s' // D3 format see https://github.com/d3/d3-format
+      title: 'Population D3'
     },
     {
       attr: 'pop',
@@ -69,13 +68,13 @@ describe('interaction popup', () => {
       expect(setContentMockClick).toHaveBeenCalledWith(`<p class="as-body">pop</p>
               <p class="as-subheader as-font--medium">10435000</p>`);
     });
-    it('should show a popup when a feature is clicked with a custom title, D3 format and custom format function', async () => {
+    it('should show a popup when a feature is clicked with a custom title, no format and custom format function', async () => {
       await layer.setPopupClick(CUSTOM_PARAM);
 
       layer.emit(InteractivityEventType.CLICK, [[FEATURE], FAKE_COORDS]);
       expect(setContentMockClick).toHaveBeenCalledWith(`<p class="as-body"></p>
               <p class="as-subheader as-font--medium">15</p><p class="as-body">Population D3</p>
-              <p class="as-subheader as-font--medium">10.435M</p><p class="as-body">Population Custom</p>
+              <p class="as-subheader as-font--medium">10435000</p><p class="as-body">Population Custom</p>
               <p class="as-subheader as-font--medium">10435K habitants</p>`);
     });
     it('should do nothing if no parameter is provided', async () => {
@@ -99,7 +98,7 @@ describe('interaction popup', () => {
 
       expect(setContentMockClick).toHaveBeenCalledWith(`<p class="as-body"></p>
               <p class="as-subheader as-font--medium">15</p><p class="as-body">Population D3</p>
-              <p class="as-subheader as-font--medium">10.435M</p><p class="as-body">Population Custom</p>
+              <p class="as-subheader as-font--medium">10435000</p><p class="as-body">Population Custom</p>
               <p class="as-subheader as-font--medium">10435K habitants</p>`);
       expect(setContentMockClick).toHaveBeenCalledTimes(1);
     });
@@ -112,7 +111,7 @@ describe('interaction popup', () => {
 
       expect(setContentMockClick).toHaveBeenCalledWith(`<p class="as-body"></p>
               <p class="as-subheader as-font--medium">15</p><p class="as-body">Population D3</p>
-              <p class="as-subheader as-font--medium">10.435M</p><p class="as-body">Population Custom</p>
+              <p class="as-subheader as-font--medium">10435000</p><p class="as-body">Population Custom</p>
               <p class="as-subheader as-font--medium">10435K habitants</p>`);
       expect(setContentMockClick).toHaveBeenCalledTimes(1);
     });
@@ -131,7 +130,7 @@ describe('interaction popup', () => {
       layer.emit(InteractivityEventType.HOVER, [[FEATURE], FAKE_COORDS]);
       expect(setContentMockHover).toHaveBeenCalledWith(`<p class="as-body"></p>
               <p class="as-subheader as-font--medium">15</p><p class="as-body">Population D3</p>
-              <p class="as-subheader as-font--medium">10.435M</p><p class="as-body">Population Custom</p>
+              <p class="as-subheader as-font--medium">10435000</p><p class="as-body">Population Custom</p>
               <p class="as-subheader as-font--medium">10435K habitants</p>`);
     });
     it('should do nothing if no parameter is provided', async () => {
@@ -155,7 +154,7 @@ describe('interaction popup', () => {
 
       expect(setContentMockHover).toHaveBeenCalledWith(`<p class="as-body"></p>
               <p class="as-subheader as-font--medium">15</p><p class="as-body">Population D3</p>
-              <p class="as-subheader as-font--medium">10.435M</p><p class="as-body">Population Custom</p>
+              <p class="as-subheader as-font--medium">10435000</p><p class="as-body">Population Custom</p>
               <p class="as-subheader as-font--medium">10435K habitants</p>`);
       expect(setContentMockHover).toHaveBeenCalledTimes(1);
     });
@@ -168,7 +167,7 @@ describe('interaction popup', () => {
 
       expect(setContentMockHover).toHaveBeenCalledWith(`<p class="as-body"></p>
               <p class="as-subheader as-font--medium">15</p><p class="as-body">Population D3</p>
-              <p class="as-subheader as-font--medium">10.435M</p><p class="as-body">Population Custom</p>
+              <p class="as-subheader as-font--medium">10435000</p><p class="as-body">Population Custom</p>
               <p class="as-subheader as-font--medium">10435K habitants</p>`);
       expect(setContentMockHover).toHaveBeenCalledTimes(1);
     });

diff --git a/src/lib/viz/popups/Popup.ts b/src/lib/viz/popups/Popup.ts
@@ -1,5 +1,4 @@
 import { Deck } from '@deck.gl/core';
-import { format as d3Format } from 'd3-format';
 import { CartoPopupError, popupErrorTypes } from '../errors/popup-error';
 import { getMapContainer } from '../utils/map-utils';
 
@@ -265,21 +264,15 @@ function generatePopupContent(elements: any, features: Record<string, any>[]): s
 
           let elementValue = feature.properties[attr];
 
-          if (format && typeof format === 'function') {
-            elementValue = format(elementValue);
-          } else if (format && typeof format === 'string') {
-            let formatter;
-
-            try {
-              formatter = d3Format(format);
-            } catch (err) {
+          if (format) {
+            if (typeof format === 'function') {
+              elementValue = format(elementValue);
+            } else {
               throw new CartoPopupError(
-                `The format '${format}' is not a recognized D3 format`,
+                `Invalid popup format: '${format}' is not a function`,
                 popupErrorTypes.FORMAT_INVALID
               );
             }
-
-            elementValue = formatter(elementValue);
           }
 
           return `<p class="as-body">${title}</p>
@@ -305,14 +298,11 @@ export interface PopupElement {
   title?: string | null;
 
   /**
-   * d3 format for the value of this attribute.
+   * Format function
    */
-  format?: string | FormatFunction;
+  format?: (value: any) => any | null;
 }
 
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-type FormatFunction = (value: any) => any;
-
 /**
  * Popup options
  */
@@ -352,17 +342,6 @@ interface PopupOptions {
     | 'bottom-right';
 }
 
-// function pixels2coordinates(pixels: number[], deckInstance?: Deck) {
-//   let coordinates;
-
-//   if (deckInstance) {
-//     const viewport = deckInstance.getViewports(undefined)[0];
-//     coordinates = viewport.unproject(pixels);
-//   }
-
-//   return coordinates;
-// }
-
 function coordinates2pixels(coordinates: number[], deckInstance?: Deck) {
   let pixels;