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

Jump to bottom

Configurable hash #8596 #8603

Merged
merged 7 commits into from
Oct 16, 2019
Merged

Configurable hash #8596 #8603

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d70a323
Configurable hash (#8596)
Aug 6, 2019
15c8213
Fix typo in doc
SebCorbin Aug 6, 2019
c2f953f
Update jsDoc
SebCorbin Aug 7, 2019
e713cfa
Reviewed per @asheemmamoowala
Aug 12, 2019
533dd54
Ensure hash is well formed
Aug 28, 2019
b361f3a
Review per @ryanhamley
Sep 4, 2019
5a1a710
Sanitize hash name input, fix flow and a bit of doc
Oct 16, 2019
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
41 changes: 36 additions & 5 deletions src/ui/hash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@ import type Map from './map';
class Hash {
_map: Map;
_updateHash: () => ?TimeoutID;
_hashName: ?string;

constructor() {
constructor(hashName: string | boolean) {
SebCorbin marked this conversation as resolved.
Show resolved Hide resolved
this._hashName = (typeof hashName === 'string' && hashName) || null;
bindAll([
'_getCurrentHash',
'_onHashChange',
'_updateHash'
], this);
Expand Down Expand Up @@ -67,18 +70,46 @@ class Hash {
if (mapFeedback) {
// new map feedback site has some constraints that don't allow
// us to use the same hash format as we do for the Map hash option.
hash += `#/${lng}/${lat}/${zoom}`;
hash += `/${lng}/${lat}/${zoom}`;
} else {
hash += `#${zoom}/${lat}/${lng}`;
hash += `${zoom}/${lat}/${lng}`;
}

if (bearing || pitch) hash += (`/${Math.round(bearing * 10) / 10}`);
if (pitch) hash += (`/${Math.round(pitch)}`);
return hash;

if (this._hashName && !mapFeedback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we just move this block into the else statement on line 74 to avoid checking mapFeedback a second time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is hardly possible, as we need bearing and pitch to be added to hash if needed. By the way, mapFeedback is not well documented so I don't understand if I need to take it into account when hashName is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that you can remove the !mapFeedback part of this check entirely without causing any issues. I'm not entirely sure what the variable is for, but this is the only reference I can find for it in the code base and it looks like it's pretty much always true if there's a hash.

let found = false;
const parts = window.location.hash.slice(1).split('&').map(part => {
const key = part.split('=')[0];
if (key === this._hashName) {
found = true;
return `${key}=${hash}`;
}
return part;
}).filter(a => a);
if (!found) {
parts.push(`${this._hashName || ''}=${hash}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the || '' part here? If there's no hash name, then shouldn't we skip the = as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test-flow fails without this, see https://circleci.com/gh/mapbox/mapbox-gl-js/50268

Cannot coerce `this._hashName` to string because null or undefined [1] should not be coerced.

   src/ui/hash.js:92:31
   92|                 parts.push(`${this._hashName}=${hash}`);
                                     ^^^^^^^^^^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

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

We can get around this by assigning this._hashName to a variable at the top of the if statement and then changing the two references to this._hashName to the variable. That lets us get rid of the unnecessary and confusing || '' part of this while keeping Flow happy.

}
return `#${parts.join('&')}`;
}

return `#${hash}`;
}

_getCurrentHash() {
const hash = window.location.hash.replace('#', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment on this just to make it a little easier to grasp at a quick glance?

if (this._hashName) {
const keyval = hash.split('&').map(
part => part.split('=')
).find(part => part[0] === this._hashName);
return (keyval ? keyval[1] || '' : '').split('/');
ryanhamley marked this conversation as resolved.
Show resolved Hide resolved
}
return hash.split('/');
}

_onHashChange() {
const loc = window.location.hash.replace('#', '').split('/');
const loc = this._getCurrentHash();
if (loc.length >= 3) {
this._map.jumpTo({
center: [+loc[2], +loc[1]],
Expand Down
5 changes: 3 additions & 2 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ type IControl = {
/* eslint-enable no-use-before-define */

type MapOptions = {
hash?: boolean,
hash?: boolean | string,
interactive?: boolean,
container: HTMLElement | string,
bearingSnap?: number,
Expand Down Expand Up @@ -173,6 +173,7 @@ const defaultOptions = {
*
* @param {boolean} [options.hash=false] If `true`, the map's position (zoom, center latitude, center longitude, bearing, and pitch) will be synced with the hash fragment of the page's URL.
SebCorbin marked this conversation as resolved.
Show resolved Hide resolved
* For example, `http://path/to/my/page.html#2.59/39.26/53.07/-24.1/60`.
* If a string is provided, it will be used as the name for a parameter-styled hash, e.g. `http://path/to/my/page.html#map=2.59/39.26/53.07/-24.1/60&foo=bar`.
ryanhamley marked this conversation as resolved.
Show resolved Hide resolved
* @param {boolean} [options.interactive=true] If `false`, no mouse, touch, or keyboard listeners will be attached to the map, so it will not respond to interaction.
* @param {number} [options.bearingSnap=7] The threshold, measured in degrees, that determines when the map's
* bearing will snap to north. For example, with a `bearingSnap` of 7, if the user rotates
Expand Down Expand Up @@ -378,7 +379,7 @@ class Map extends Camera {

bindHandlers(this, options);

this._hash = options.hash && (new Hash()).addTo(this);
this._hash = options.hash && (new Hash(options.hash)).addTo(this);
// don't set position from options if set through hash
if (!this._hash || !this._hash._onHashChange()) {
this.jumpTo({
Expand Down
109 changes: 107 additions & 2 deletions test/unit/ui/hash.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import window from '../../../src/util/window';
import { createMap as globalCreateMap } from '../../util';

test('hash', (t) => {
function createHash() {
const hash = new Hash();
function createHash(name) {
SebCorbin marked this conversation as resolved.
Show resolved Hide resolved
const hash = new Hash(name);
hash._updateHash = hash._updateHashUnthrottled.bind(hash);
return hash;
}
Expand Down Expand Up @@ -100,6 +100,70 @@ test('hash', (t) => {
t.end();
});

t.test('#_onHashChange named', (t) => {
const map = createMap(t);
const hash = createHash('map')
.addTo(map);

window.location.hash = '#map=10/3.00/-1.00&foo=bar';

hash._onHashChange();

t.equal(map.getCenter().lng, -1);
t.equal(map.getCenter().lat, 3);
t.equal(map.getZoom(), 10);
t.equal(map.getBearing(), 0);
t.equal(map.getPitch(), 0);

window.location.hash = '';

t.end();
});

t.test('#_getCurrentHash', (t) => {
const map = createMap(t);
const hash = createHash()
.addTo(map);

window.location.hash = '#10/3.00/-1.00';

const currentHash = hash._getCurrentHash();

t.equal(currentHash[0], '10');
t.equal(currentHash[1], '3.00');
t.equal(currentHash[2], '-1.00');

window.location.hash = '';

t.end();
});

t.test('#_getCurrentHash named', (t) => {
const map = createMap(t);
const hash = createHash('map')
.addTo(map);

window.location.hash = '#map=10/3.00/-1.00&foo=bar';

let currentHash = hash._getCurrentHash();

t.equal(currentHash[0], '10');
t.equal(currentHash[1], '3.00');
t.equal(currentHash[2], '-1.00');

window.location.hash = '#baz&map=10/3.00/-1.00';

currentHash = hash._getCurrentHash();

t.equal(currentHash[0], '10');
t.equal(currentHash[1], '3.00');
t.equal(currentHash[2], '-1.00');

window.location.hash = '';

t.end();
});

t.test('#_updateHash', (t) => {
function getHash() {
return window.location.hash.split('/');
Expand Down Expand Up @@ -145,6 +209,47 @@ test('hash', (t) => {
t.equal(newHash[3], '135');
t.equal(newHash[4], '60');

window.location.hash = '';

t.end();
});

t.test('#_updateHash named', (t) => {
const map = createMap(t);
createHash('map')
.addTo(map);

t.notok(window.location.hash);

map.setZoom(3);
map.setCenter([1.0, 2.0]);

t.ok(window.location.hash);

t.equal(window.location.hash, '#map=3/2/1');

map.setPitch(60);

t.equal(window.location.hash, '#map=3/2/1/0/60');

map.setBearing(135);

t.equal(window.location.hash, '#map=3/2/1/135/60');

window.location.hash += '&foo=bar';

map.setZoom(7);

t.equal(window.location.hash, '#map=7/2/1/135/60&foo=bar');

window.location.hash = '#baz&map=7/2/1/135/60&foo=bar';

map.setCenter([2.0, 1.0]);

t.equal(window.location.hash, '#baz&map=7/1/2/135/60&foo=bar');

window.location.hash = '';

t.end();
});

Expand Down