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.

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

Merged
merged 1 commit into from Oct 8, 2018
Merged

API skeleton #4290

merged 1 commit into from Oct 8, 2018

Conversation

fredj
Copy link
Member

@fredj fredj commented Oct 5, 2018

No description provided.

@fredj fredj changed the title [wip] API skeleton API skeleton Oct 8, 2018
@fredj
Copy link
Member Author

fredj commented Oct 8, 2018

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';
Copy link
Member

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@ger-benjamin ger-benjamin left a 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
Copy link
Member

Choose a reason for hiding this comment

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

ids ?

Copy link
Member Author

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,
Copy link
Member

Choose a reason for hiding this comment

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

options.zoom = 10 ?

Copy link
Member Author

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') {
Copy link
Member

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)

Copy link
Member Author

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'));
Copy link
Member

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 ?)

Copy link
Member Author

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

Copy link
Member

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

@ger-benjamin
Copy link
Member

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",
Copy link
Member

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?

@sbrunner sbrunner added this to the 2.4 milestone Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants