-
Notifications
You must be signed in to change notification settings - Fork 87
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.
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
API skeleton #4290
API skeleton #4290
Conversation
Not complete yet but merging as step 1 |
api/Map.js
Outdated
|
||
// FIXME: temporary | ||
import SwisstopoSource from '@geoblocks/sources/Swisstopo.js'; | ||
import EPSG_21781 from '@geoblocks/sources/EPSG21781.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use EPSG21781
as in the rest of the project (to search and replace in the future for instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do a quick improve of the indentation in file api/dist/apihelp.html ?
That's confusing
You can also merge directly if you think it's okay in the limit of this project
api/Map.js
Outdated
|
||
/** | ||
* @param {string} layer | ||
* @param {Array.<string>} layer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ids ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oups, thanks!
api/Map.js
Outdated
this.view_ = new View({ | ||
projection: getProjection(constants.projection || 'EPSG:21781'), | ||
resolutions: constants.resolutions, | ||
zoom: options.zoom, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options.zoom = 10 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
api/Map.js
Outdated
* @property {function()} [error] | ||
*/ | ||
addCustomLayer(type, name, url, options = {}) { | ||
if (type === 'text') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no else
? So perhaps you can avoid this parameter ? (or add a FIXME if you plan to support more types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old api supports text and gpx but the new version only supports text.
Because we need to be compatible with the old api, the type must stay; I've removed the if
api/Map.js
Outdated
const columns = lines.shift().split('\t'); | ||
for (const line of lines) { | ||
if (line) { | ||
const values = zip(columns, line.split('\t')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It is cross compatible ? It is always \t
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's always \t
. It was like this in openlayers 2:
https://github.com/openlayers/ol2/blob/master/lib/OpenLayers/Format/Text.js#L91
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I was just suspicious
Please feel free to merge |
@@ -22,6 +24,7 @@ | |||
"compile-catalog": "buildtools/compile-catalog.js" | |||
}, | |||
"devDependencies": { | |||
"@geoblocks/sources": "https://api.github.com/repos/geoblocks/sources/tarball/173ad0171", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you add a ticket to remove this?
No description provided.