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 11 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
26 changes: 20 additions & 6 deletions src/ui/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,13 @@ export default class Popup extends Evented {
_lngLat: LngLat;
_trackPointer: boolean;
_pos: ?Point;
_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.split(` `) : []);
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -435,30 +437,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} `this`
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
*
* @example
* let popup = new mapboxgl.Popup()
* popup.addClassName('some-class')
*/
addClassName(className: string) {
this._classList.add(className);
if (this._container) {
this._container.classList.add(className);
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about overwriting classes with something like this._applyClassList() which would look like:

_applyClassList () {
  if (!this._container) return;
  this._container.className = [...this._classList.values()].join(' ');
}

My main concern is that we have two independent sources of truth which we hope remain synchronized. But since we don't allow a user-supplied container anyway, so I don't think there should be a case where this would prevent you from accomplishing some behavior you couldn't simply achieve by using the supplied API.

However, this could be a breaking change if someone was previously manipulating the container class directly.

Copy link
Contributor Author

@SnailBones SnailBones Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there's a valid reason for anyone to manipulate the container class directly right now. (Still doesn't mean that no one's doing it.)

It wouldn't be quite as simple as overwriting: the container element has additional default classes. So this:

 const popup = new Popup({className: 'some classes'});

creates this element:

 <div class="mapboxgl-popup mapboxgl-popup-anchor-bottom yellow"></div>

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! Just wanted to be sure.

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 did find one weird edge case: if the user toggles one of the default classes:

popup.toggleClassName('mapboxgl-popup');

This removes the item from the container while adding it to the list.

I'm sure we could fix this, but I'm not sure what the correct behavior would be or if it's worth the lift and added complexity just to solve a very specific edge case.

}
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} `this`
* @example
* let 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);
}
return this;
}

/**
Expand All @@ -482,12 +490,18 @@ export default class Popup extends Evented {
*
* @example
* let popup = new mapboxgl.Popup()
* popup.toggleClassName('toggleClass')
* popup.toggleClassName('highlighted')
*/
toggleClassName(className: string) {
if (this._container) {
return this._container.classList.toggle(className);
this._container.classList.toggle(className);
}
if (this._classList.has(className)) {
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
this._classList.delete(className);
return false;
}
this._classList.add(className);
return true;
}

_createCloseButton() {
Expand Down Expand Up @@ -521,8 +535,8 @@ 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 =>
if (this._classList) {
this._classList.forEach(name =>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we overwrote classes, then I think this._container.className = [...this._classList.values()].join(' ') would be the simplest way to accomplish this.

this._container.classList.add(name));
}

Expand Down
68 changes: 68 additions & 0 deletions test/unit/ui/popup.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,74 @@ test('Popup adds classes from className option, methods for class manipulations
t.end();
});

test('Popup.addClassName adds classes when called before adding popup to map', (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 manipulations works 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.ok(popupContainer.classList.contains('toggle'));

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

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

test('Cursor-tracked popup disappears on mouseout', (t) => {
const map = createMap(t);

Expand Down