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

Allow edits to popup container class while the popup is not on the map #10889

Merged
merged 19 commits into from
Aug 2, 2021
Merged

Allow edits to popup container class while the popup is not on the map #10889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4bd9639
addClassName method can be called before the popup is on map
SnailBones Jul 21, 2021
4b98354
Added tests
SnailBones Jul 21, 2021
1e2f063
Updating incorrect examples
SnailBones Jul 21, 2021
c523598
Added console.warn error messages for incorrect function use
SnailBones Jul 21, 2021
051be6b
Pin chrome to version 91 (#10887)
Jul 20, 2021
646002a
Update src/ui/popup.js
SnailBones Jul 21, 2021
bca4cfb
Fixed asterisks for jsdoc
SnailBones Jul 21, 2021
aba074b
Removing else with early return
SnailBones Jul 21, 2021
774d41e
Updates to `flyTo` docs (#10890)
HeyStenson Jul 21, 2021
f67bb87
Added _classList Set to Popup
SnailBones Jul 22, 2021
0c3349e
Merge branch 'main' into aidan/popup-class-name
SnailBones Jul 22, 2021
da5268b
Trim and split on any whitespace as Ricky suggested
SnailBones Jul 22, 2021
acfa889
Update documentation for @returns
SnailBones Jul 22, 2021
5d7b61d
Refactored so that changes to the container are always performed by _…
SnailBones Jul 23, 2021
b9e20fa
Inlining function from anchor.js to marker.js
SnailBones Jul 24, 2021
fd70ead
Typo fix
SnailBones Jul 25, 2021
aa89bc2
Small changes from suggestions
SnailBones Aug 2, 2021
3e0a353
Merge branch 'main' into aidan/popup-class-name
SnailBones Aug 2, 2021
745581b
Aadding ;s to examples
SnailBones Aug 2, 2021
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
8 changes: 0 additions & 8 deletions src/ui/anchor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,3 @@ export const anchorTranslate: {[_: Anchor]: string} = {
'left': 'translate(0,-50%)',
'right': 'translate(-100%,-50%)'
};

export function applyAnchorClass(element: HTMLElement, anchor: Anchor, prefix: string) {
const classList = element.classList;
for (const key in anchorTranslate) {
classList.remove(`mapboxgl-${prefix}-anchor-${key}`);
}
classList.add(`mapboxgl-${prefix}-anchor-${anchor}`);
}
8 changes: 6 additions & 2 deletions src/ui/marker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import LngLat from '../geo/lng_lat.js';
import Point from '@mapbox/point-geometry';
import smartWrap from '../util/smart_wrap.js';
import {bindAll, extend} from '../util/util.js';
import {type Anchor, anchorTranslate, applyAnchorClass} from './anchor.js';
import {type Anchor, anchorTranslate} from './anchor.js';
import {Event, Evented} from '../util/evented.js';
import type Map from './map.js';
import type Popup from './popup.js';
Expand Down Expand Up @@ -233,7 +233,11 @@ export default class Marker extends Evented {
// prevent focusing on click
e.preventDefault();
});
applyAnchorClass(this._element, this._anchor, 'marker');
const classList = this._element.classList;
for (const key in anchorTranslate) {
classList.remove(`mapboxgl-marker-anchor-${key}`);
}
classList.add(`mapboxgl-marker-anchor-${this._anchor}`);

this._popup = null;
}
Expand Down
142 changes: 79 additions & 63 deletions src/ui/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import LngLat from '../geo/lng_lat.js';
import Point from '@mapbox/point-geometry';
import window from '../util/window.js';
import smartWrap from '../util/smart_wrap.js';
import {type Anchor, anchorTranslate, applyAnchorClass} from './anchor.js';
import {type Anchor, anchorTranslate} from './anchor.js';

import type Map from './map.js';
import type {LngLatLike} from '../geo/lng_lat.js';
Expand Down Expand Up @@ -109,11 +109,15 @@ export default class Popup extends Evented {
_lngLat: LngLat;
_trackPointer: boolean;
_pos: ?Point;
_anchor: Anchor;
_classList: Set<string>;

constructor(options: PopupOptions) {
super();
this.options = extend(Object.create(defaultOptions), options);
bindAll(['_update', '_onClose', 'remove', '_onMouseMove', '_onMouseUp', '_onDrag'], this);
this._classList = new Set(options && options.className ?
options.className.trim().split(/\s+/) : []);
}

/**
Expand Down Expand Up @@ -150,9 +154,6 @@ export default class Popup extends Evented {
if (this._trackPointer) {
this._map.on('mousemove', this._onMouseMove);
this._map.on('mouseup', this._onMouseUp);
if (this._container) {
this._container.classList.add('mapboxgl-popup-track-pointer');
}
this._map._canvasContainer.classList.add('mapboxgl-track-pointer');
} else {
this._map.on('move', this._update);
Expand Down Expand Up @@ -280,9 +281,6 @@ export default class Popup extends Evented {
if (this._map) {
this._map.on('move', this._update);
this._map.off('mousemove', this._onMouseMove);
if (this._container) {
this._container.classList.remove('mapboxgl-popup-track-pointer');
}
this._map._canvasContainer.classList.remove('mapboxgl-track-pointer');
}

Expand All @@ -308,9 +306,6 @@ export default class Popup extends Evented {
this._map.off('move', this._update);
this._map.on('mousemove', this._onMouseMove);
this._map.on('drag', this._onDrag);
if (this._container) {
this._container.classList.add('mapboxgl-popup-track-pointer');
}
this._map._canvasContainer.classList.add('mapboxgl-track-pointer');
}

Expand Down Expand Up @@ -451,30 +446,36 @@ export default class Popup extends Evented {
* Adds a CSS class to the popup container element.
*
* @param {string} className Non-empty string with CSS class name to add to popup container.
* @returns {Popup} Returns itself to allow for method chaining.
*
* @example
* const popup = new mapboxgl.Popup();
* popup.addClassName('some-class');
*/
addClassName(className: string) {
this._classList.add(className);
if (this._container) {
this._container.classList.add(className);
this._updateClassList();
}
return this;
}

/**
* Removes a CSS class from the popup container element.
*
* @param {string} className Non-empty string with CSS class name to remove from popup container.
*
* @returns {Popup} Returns itself to allow for method chaining.
* @example
* const popup = new mapboxgl.Popup();
* popup.removeClassName('some-class');
* const popup = new mapboxgl.Popup({className: 'some classes'});
* popup.removeClassName('some');
*/
removeClassName(className: string) {
this._classList.delete(className);
if (this._container) {
this._container.classList.remove(className);
this._updateClassList();
}
return this;
}

/**
Expand Down Expand Up @@ -512,12 +513,20 @@ export default class Popup extends Evented {
*
* @example
* const popup = new mapboxgl.Popup();
* popup.toggleClassName('toggleClass');
* popup.toggleClassName('highlighted');
*/
toggleClassName(className: string) {
let finalState: boolean;
if (this._classList.delete(className)) {
finalState = false;
} else {
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
this._classList.add(className);
finalState = true;
}
if (this._container) {
return this._container.classList.toggle(className);
this._updateClassList();
}
return finalState;
}

_createCloseButton() {
Expand All @@ -542,6 +551,47 @@ export default class Popup extends Evented {
this._update(event.point);
}

_getAnchor(offset: any) {
if (this.options.anchor) { return this.options.anchor; }

const pos: any = this._pos;
const width = this._container.offsetWidth;
const height = this._container.offsetHeight;
let anchorComponents;

if (pos.y + offset.bottom.y < height) {
anchorComponents = ['top'];
} else if (pos.y > this._map.transform.height - height) {
rreusser marked this conversation as resolved.
Show resolved Hide resolved
anchorComponents = ['bottom'];
} else {
anchorComponents = [];
}

if (pos.x < width / 2) {
anchorComponents.push('left');
} else if (pos.x > this._map.transform.width - width / 2) {
anchorComponents.push('right');
}

if (anchorComponents.length === 0) {
return 'bottom';
}
return ((anchorComponents.join('-'): any): Anchor);

}

_updateClassList() {
const classes = [...this._classList];
classes.push('mapboxgl-popup');
if (this._anchor) {
classes.push(`mapboxgl-popup-anchor-${this._anchor}`);
}
if (this._trackPointer) {
classes.push('mapboxgl-popup-track-pointer');
}
this._container.className = classes.join(' ');
}

_update(cursor: ?PointLike) {
const hasPosition = this._lngLat || this._trackPointer;

Expand All @@ -551,14 +601,6 @@ export default class Popup extends Evented {
this._container = DOM.create('div', 'mapboxgl-popup', this._map.getContainer());
this._tip = DOM.create('div', 'mapboxgl-popup-tip', this._container);
this._container.appendChild(this._content);
if (this.options.className) {
this.options.className.split(' ').forEach(name =>
this._container.classList.add(name));
}

if (this._trackPointer) {
this._container.classList.add('mapboxgl-popup-track-pointer');
}
}

if (this.options.maxWidth && this._container.style.maxWidth !== this.options.maxWidth) {
Expand All @@ -569,46 +611,21 @@ export default class Popup extends Evented {
this._lngLat = smartWrap(this._lngLat, this._pos, this._map.transform);
}

if (this._trackPointer && !cursor) return;
if (!this._trackPointer || cursor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Is there a reason for inverting the conditional? Early return can be nice to de-nest code when otherwise equivalent. Nothing blocking here for me though.

Copy link
Contributor Author

@SnailBones SnailBones Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking was to remove the early return allowing this. _updateClassList() to be called in any condition. I realize there's another call inside here, I'll double-check and see if I can remove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup it was redundant! (Oops) Fixed in latest commit.

const pos = this._pos = this._trackPointer && cursor ? cursor : this._map.project(this._lngLat);

const pos = this._pos = this._trackPointer && cursor ? cursor : this._map.project(this._lngLat);
const offset = normalizeOffset(this.options.offset);
const anchor = this._anchor = this._getAnchor(offset);

let anchor: ?Anchor = this.options.anchor;
const offset = normalizeOffset(this.options.offset);

if (!anchor) {
const width = this._container.offsetWidth;
const height = this._container.offsetHeight;
let anchorComponents;

if (pos.y + offset.bottom.y < height) {
anchorComponents = ['top'];
} else if (pos.y > this._map.transform.height - height) {
anchorComponents = ['bottom'];
} else {
anchorComponents = [];
}

if (pos.x < width / 2) {
anchorComponents.push('left');
} else if (pos.x > this._map.transform.width - width / 2) {
anchorComponents.push('right');
}

if (anchorComponents.length === 0) {
anchor = 'bottom';
} else {
anchor = (anchorComponents.join('-'): any);
}
const offsetedPos = pos.add(offset[anchor]).round();
this._map._requestDomTask(() => {
if (this._container && anchor) {
DOM.setTransform(this._container, `${anchorTranslate[anchor]} translate(${offsetedPos.x}px,${offsetedPos.y}px)`);
}
});
}

const offsetedPos = pos.add(offset[anchor]).round();
this._map._requestDomTask(() => {
if (this._container && anchor) {
DOM.setTransform(this._container, `${anchorTranslate[anchor]} translate(${offsetedPos.x}px,${offsetedPos.y}px)`);
applyAnchorClass(this._container, anchor, 'popup');
}
});
this._updateClassList();
}

_focusFirstElement() {
Expand All @@ -630,10 +647,9 @@ export default class Popup extends Evented {
}

function normalizeOffset(offset: ?Offset) {
if (!offset) {
return normalizeOffset(new Point(0, 0));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seemed like a strange case for recursion here

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe I've seen this style, especially in the symbol code, where it runs modified values through a different branch of the same function, but TBH I find it slightly hard to follow, so that if otherwise equivalent, I prefer your change 👍

if (!offset) offset = (new Point(0, 0));

} else if (typeof offset === 'number') {
if (typeof offset === 'number') {
// input specifies a radius from which to calculate offsets at all positions
const cornerOffset = Math.round(Math.sqrt(0.5 * Math.pow(offset, 2)));
return {
Expand Down
74 changes: 67 additions & 7 deletions test/unit/ui/popup.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ test('Popup#remove is idempotent (#2395)', (t) => {
t.end();
});

test('Popup adds classes from className option, methods for class manipulations works properly', (t) => {
test('Popup adds classes from className option, methods for class manipulation work properly', (t) => {
const map = createMap(t);
const popup = new Popup({className: 'some classes'})
.setText('Test')
Expand All @@ -599,14 +599,74 @@ test('Popup adds classes from className option, methods for class manipulations
popup.toggleClassName('toggle');
t.ok(!popupContainer.classList.contains('toggle'));

t.throws(() => popup.addClassName('should throw exception'), window.DOMException);
t.throws(() => popup.removeClassName('should throw exception'), window.DOMException);
t.throws(() => popup.toggleClassName('should throw exception'), window.DOMException);
t.end();
});

test('Popup#addClassName adds classes when called before adding popup to map (#9677)', (t) => {
const map = createMap(t);
const popup = new Popup();
popup.addClassName('some');
popup.addClassName('classes');

popup.setText('Test')
.setLngLat([0, 0])
.addTo(map);

const popupContainer = popup.getElement();
t.ok(popupContainer.classList.contains('some'));
t.ok(popupContainer.classList.contains('classes'));
t.end();
});
test('Popup className option and addClassName both add classes', (t) => {
const map = createMap(t);
const popup = new Popup({className: 'some classes'});
popup.addClassName('even')
.addClassName('more');

popup.setText('Test')
.setLngLat([0, 0])
.addTo(map);

popup.addClassName('one-more');

const popupContainer = popup.getElement();
t.ok(popupContainer.classList.contains('some'));
t.ok(popupContainer.classList.contains('classes'));
t.ok(popupContainer.classList.contains('even'));
t.ok(popupContainer.classList.contains('more'));
t.ok(popupContainer.classList.contains('one-more'));
t.end();
});

test('Methods for class manipulation work properly when popup is not on map', (t) => {
const map = createMap(t);
const popup = new Popup()
.setText('Test')
.setLngLat([0, 0])
.addClassName('some')
.addClassName('classes');

let popupContainer = popup.addTo(map).getElement();
t.ok(popupContainer.classList.contains('some'));
t.ok(popupContainer.classList.contains('classes'));

popup.remove();
popup.removeClassName('some');
popupContainer = popup.addTo(map).getElement();

t.ok(!popupContainer.classList.contains('some'));

popup.remove();
popup.toggleClassName('toggle');
popupContainer = popup.addTo(map).getElement();

t.throws(() => popup.addClassName(''), window.DOMException);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since DOM is no longer directly updated, these functions no longer throw an error.

t.throws(() => popup.removeClassName(''), window.DOMException);
t.throws(() => popup.toggleClassName(''), window.DOMException);
t.ok(popupContainer.classList.contains('toggle'));

popup.remove();
popup.toggleClassName('toggle');
popupContainer = popup.addTo(map).getElement();

t.ok(!popupContainer.classList.contains('toggle'));
t.end();
});

Expand Down