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.

Sign up for GitHub

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

Remove d3 format option from PopUps #68

Merged
merged 6 commits into from
Jul 9, 2020
Merged

Remove d3 format option from PopUps #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8f6292
feat: remove d3.format dependency
VictorVelarde Jul 7, 2020
ac6e357
test: remove d3.format
VictorVelarde Jul 7, 2020
20c869b
fix: recover format property for popup
VictorVelarde Jul 8, 2020
62757d7
refactor: format is not a function error
VictorVelarde Jul 8, 2020
ff306ae
chore: add description to changelog
VictorVelarde Jul 8, 2020
269af53
chore: fix typo
VictorVelarde Jul 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
-
Expand Down
2 changes: 1 addition & 1 deletion examples/_debug/widgets/geojson-formula-data-view-all-modes.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion examples/dataview/formula-data-view-all-modes.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion examples/dataview/formula-data-view.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
6 changes: 2 additions & 4 deletions examples/interactivity/add-popups-onhover.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 4 additions & 6 deletions examples/interactivity/popups-format.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down Expand Up @@ -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',
Expand Down
13 changes: 1 addition & 12 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
17 changes: 8 additions & 9 deletions src/lib/viz/__tests__/interactivity-popups.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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 () => {
Expand All @@ -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);
});
Expand All @@ -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);
});
Expand All @@ -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 () => {
Expand All @@ -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);
});
Expand All @@ -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);
});
Expand Down
35 changes: 7 additions & 28 deletions src/lib/viz/popups/Popup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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>
Expand All @@ -305,14 +298,11 @@ export interface PopupElement {
title?: string | null;

/**
* d3 format for the value of this attribute.
* Format function
*/
format?: string | FormatFunction;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should keep the format option for functions right?

format?: (value: any) => any;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was just related to d3... oka

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup sorry, the comment was outdated and wrong 😅

format?: (value: any) => any | null;
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type FormatFunction = (value: any) => any;

/**
* Popup options
*/
Expand Down Expand Up @@ -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;

Expand Down