Read zoom and attribution config from /<style>/info.json #5

mattflaschen · 2018-02-13T07:13:38Z

This allows different installs to use their own setting while
reducing hard-coding.

When you have URL parameters, there is currently a flash where you see
the default map before it loads the settings and zooms. The settings
could be cached in localStorage to mitigate that.

This changes the default max zoom (used only if the /info.json request
fails) to 18. This is on the theory that 18 is safe if 20 is
available, but not vice-versa.

Bug: T186780

Updated version of #4 .

mattflaschen · 2018-02-13T07:14:41Z

I think 18 is a better (safer) default, but I don't feel strongly about that.

I'm not sure if the flash is a blocker.

nyurik · 2018-02-13T07:19:18Z

static/main.js

+		maxZoom: config.maxzoom ? config.maxzoom : 18,
+
+		// TODO: This is UI text, and needs to be translatable.
+		attribution: config.attribution ? config.attribution : 'Map data &copy; <a href="http://openstreetmap.org/copyright">OpenStreetMap contributors</a>',


How about config.attribution || 'Map data © <a href="/load/view.php?a=aHR0cDovL29wZW5zdHJlZXRtYXAub3JnL2NvcHlyaWdodA">OpenStreetMap contributors</a>' -- seems to be cleaner? Same for maxzoom. This is a fairly common pattern in JS.

I would even offer, for the sake of organization, to create a "defaultSettings" object with default values at the top, and then do something like

layerSettings = Object.assign( {}, defaultSettings, config );

OR if you want to make sure you only take the specific config options, do something like

layerSettings = Object.assign( {}, defaultSettings, {
maxZoom: config.maxzoom,
attribution: config.attribution
} );

etc... this can make the code more readable and, if more config options are added later, it makes it easier to immediately spot where to add defaults, etc.

Also, we are assuming that 'attribution' cannot be an empty string? I am not against it, but we do need to acknowledge that. If we want to allow empty string, we should do something like

attribution: config.attribution !== undefined ? config.attribution : 'default value goes here';

or something like that. It might be better to not allow for an empty attribution if we're sure all sources are from OpenStreetMap, but if anyone uses this library to pull from anything else and doesn't want to put attribution (and it's legal) then empty string may be valid. Just raising this for consideration. @nyurik what do you think?

All good points, but I can't do both (unless I use different loops for maxzoom and attribution).

We can't use Object.assign yet (IE 10 and 11). It's probably better not to anyway, due to the 'specific config options' issue you mention (this is all public data, but Leaflet may not interpret field names the same way; there is a leaflet-tilejson, but we're not using it).

I believe all of the WMF tiles use OSM, which requires attribution.

However, there are public domain sources, e.g. Natural Earth, so in principle some user of this code (and different tiles) might not want attribution. This is an edge case, but I went with your suggestion. That means I can't use ||.

nyurik · 2018-02-13T07:20:47Z

minor thing, but looks good otherwise. What is a "flash is a blocker"?

mooeypoo · 2018-02-13T18:26:38Z

static/main.js

+	});
+
+	// Add current location to URL hash
+	var hash = new L.Hash(map);


Put all 'var xxx' up top

This was already like that. However, I should move it like the others.

Actually, it's never used.

nyurik · 2018-02-13T19:40:29Z

@mooeypoo thx, but just to keep it in perspective - this is a test file, used for testing that the maps server is still up and running, so we shouldn't spend too much time on it :) Most of the client side polishing should be probably done in the kartographer ext.

mattflaschen · 2018-02-14T01:07:18Z

minor thing, but looks good otherwise. What is a "flash is a blocker"?

See second paragraph of the commit message. Essentially, if you open a URL like https://maps.wikimedia.org/#8/41.865/-93.043 in a new tab (or just start there and refresh), you'll see a flash of the entire North American continent before it goes where you requested. I thought this was caused by my patch, but now I see it's happening on maps.wikimedia.org already. And logically, the patch shouldn't make the flash worse (though overall time might get a little worse), since it doesn't even register the layer/tile URL (so can't show the default layer view/continent) before the AJAX request is already done.

I think the flash is worse locally generally.

mattflaschen · 2018-02-14T01:14:41Z

@mooeypoo thx, but just to keep it in perspective - this is a test file, used for testing that the maps server is still up and running, so we shouldn't spend too much time on it :) Most of the client side polishing should be probably done in the kartographer ext.

I agree there's no need to overdo it, but it's at a very prominent URL (maps.wikimedia.org), so it should be solid (solid and simple is good, though).

nyurik · 2018-02-14T01:52:32Z

@mattflaschen sounds all good. Let me know if you want to clean it up a bit, or if not, I'm ok to merge it even as is.

mattflaschen · 2018-02-14T02:38:24Z

I'll have the changes I mentioned above uploaded today. It will need a rebase after that (I thought I got it working on top of master, but I was wrong).

It will be on 0.0.17 again, the version from https://github.com/kartotherian/kartotherian/blob/update-snapshot-dep/package.json . Someone needs to look at how to get the overall stack working with a newer kartotherian/server.

We can't just repoint kartotherian/kartotherian package.json to kartotherian/server master + this patch (at least for me, an equivalent to that causes "source.getHandler(...).getAsync is not a function")

When the overall stack is working, this can be merged and tagged, then kartotherian/kartotherian package.json updated.

This allows different installs to use their own setting while reducing hard-coding. This changes the default max zoom (used only if the info.json request fails) to 18. This is on the theory that 18 is safe if 20 is available, but not vice-versa. This is based off a tag (0.0.17), since that is the only way I can get it to work with update-snapshot-dep. It needs to be rebased. Bug: T186780

mattflaschen · 2018-02-14T03:18:36Z

It's only marked WIP because of the 0.0.17 issue I noted above (master + this patch needs to be successfully tested with the rest of the stack).

mattflaschen · 2018-02-15T00:57:01Z

Getting the repos working together with latest kartotherian/server tracked as https://phabricator.wikimedia.org/T187411 .

stephanebisson · 2018-03-02T15:02:35Z

This PR should now be rebased on top of master.

mooeypoo · 2018-03-02T19:26:58Z

Note: The current PR fails, and is also not listening to the proper browser eslint rules. I'm fixing this, please do not merge yet.

nyurik · 2018-03-04T00:24:57Z

@mooeypoo btw, if you want, we can make the static content fully wikimedia-only style -- simply because it is totally a standalone, and used just for testing. I'm more concerned about all the other nodejs code.

mooeypoo · 2018-03-04T22:40:51Z

Yeah, in order to solve the linting and es6 issues that don't work on browser, I submitted #8 which basically uses the wikimedia style inside the static folder. I just couldn't use airbnb styling, since all their rules are es6 and even if we override the environment, they still stick ...

nyurik · 2018-03-04T22:48:04Z

I've merged #8, let me know when this PR can be merged.

mooeypoo · 2018-03-06T18:50:37Z

I verified that this PR works when running with https://github.com/kartotherian/kartotherian/tree/update-deps2

It's good to go.

nyurik reviewed Feb 13, 2018

View reviewed changes

mooeypoo reviewed Feb 13, 2018

View reviewed changes

mattflaschen force-pushed the dynamicMaxZoom branch from 5a2bd58 to 8865aa0 Compare February 14, 2018 03:14

Merge branch 'master' into dynamicMaxZoom

da29ebd

Merge branch 'master' into dynamicMaxZoom

2e3064d

nyurik merged commit e2802b9 into kartotherian:master Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read zoom and attribution config from /<style>/info.json #5

Read zoom and attribution config from /<style>/info.json #5

mattflaschen commented Feb 13, 2018

mattflaschen commented Feb 13, 2018

nyurik Feb 13, 2018

mooeypoo Feb 13, 2018

mooeypoo Feb 13, 2018

mattflaschen Feb 14, 2018

nyurik commented Feb 13, 2018

mooeypoo Feb 13, 2018

mattflaschen Feb 13, 2018

mattflaschen Feb 14, 2018

nyurik commented Feb 13, 2018

mattflaschen commented Feb 14, 2018

mattflaschen commented Feb 14, 2018

nyurik commented Feb 14, 2018

mattflaschen commented Feb 14, 2018

mattflaschen commented Feb 14, 2018

mattflaschen commented Feb 15, 2018

stephanebisson commented Mar 2, 2018

mooeypoo commented Mar 2, 2018

nyurik commented Mar 4, 2018

mooeypoo commented Mar 4, 2018

nyurik commented Mar 4, 2018

mooeypoo commented Mar 6, 2018

Read zoom and attribution config from /<style>/info.json #5

Read zoom and attribution config from /<style>/info.json #5

Conversation

mattflaschen commented Feb 13, 2018

mattflaschen commented Feb 13, 2018

nyurik Feb 13, 2018

Choose a reason for hiding this comment

mooeypoo Feb 13, 2018

Choose a reason for hiding this comment

mooeypoo Feb 13, 2018

Choose a reason for hiding this comment

mattflaschen Feb 14, 2018

Choose a reason for hiding this comment

nyurik commented Feb 13, 2018

mooeypoo Feb 13, 2018

Choose a reason for hiding this comment

mattflaschen Feb 13, 2018

Choose a reason for hiding this comment

mattflaschen Feb 14, 2018

Choose a reason for hiding this comment

nyurik commented Feb 13, 2018

mattflaschen commented Feb 14, 2018

mattflaschen commented Feb 14, 2018

nyurik commented Feb 14, 2018

mattflaschen commented Feb 14, 2018

mattflaschen commented Feb 14, 2018

mattflaschen commented Feb 15, 2018

stephanebisson commented Mar 2, 2018

mooeypoo commented Mar 2, 2018

nyurik commented Mar 4, 2018

mooeypoo commented Mar 4, 2018

nyurik commented Mar 4, 2018

mooeypoo commented Mar 6, 2018