Added abitlity to load from an already loaded file content (e.g. loaded from URL) #34
Conversation
- FileLoader.loadData(data, name, ext)
where:
data is the input data to load (a GeoJson, GPX or KML)
name is the name of the data (file name, URL, etc)
ext is an optional parameter to set data type in order to force parser type
- fire 'data:error' if load/loadData mandatory parameters are missing
- trick to allow testing of load file (set file.testing to true to skip reader load)
- tests for loadData
opoto
changed the title
There was a problem hiding this comment.
Hi @opoto ! Thank you a lot for this contribution ! We really appreciate it.
If you don't mind I've put some review on few syntax things. The overall is really good, those are more to keep following our .eslintrc file. If you want you can enable eslint in your IDE for this project, rules are already in dev dependencies. Anyway, I just put the matching information on the pull request code.
Don't worry about the tests respecting the linter as I need to clean them anyway.
If you have any question or doubt, please let me know, I will be glad to help or exchange about it.
| @@ -63,46 +63,117 @@ | |||
| var ext; | |||
There was a problem hiding this comment.
ext var is not used anymore. It should be deleted
| this.fire('data:loaded', { layer: layer, filename: file.name, format: ext }); | ||
| this.fire('data:loading', { filename: file.name, format: parser.ext }); | ||
| layer = parser.processor.call(this, e.target.result, parser.ext); | ||
| this.fire('data:loaded', { layer: layer, filename: file.name, format: parser.ext }); |
There was a problem hiding this comment.
The line 90 exceeds the 100 char, please pass the options object on multiple lines
| } catch (err) { | ||
| this.fire('data:error', { error: err }); | ||
| } | ||
| }, this); | ||
| reader.readAsText(file); | ||
|
|
||
| // Testing trick: tests don't pass a real file, but an object with file.testing set to true. |
There was a problem hiding this comment.
A trailing space to remove at the end of the line 95 :)
| var parser; | ||
|
|
||
| // Check required parameters | ||
| if ((this._isParameterMissing(data, "data")) |
There was a problem hiding this comment.
Please use single quotes for string
|
|
||
| // Check required parameters | ||
| if ((this._isParameterMissing(data, "data")) | ||
| || (this._isParameterMissing(name, "name"))) { |
There was a problem hiding this comment.
Please use single quotes for string
| } catch (err) { | ||
| this.fire('data:error', { error: err }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove line 133 - block should not be padded with blank lines
|
|
||
| }, | ||
|
|
||
| _isParameterMissing: function (v, vname, vtype) { |
There was a problem hiding this comment.
vtype should be deleted - vtype is defined but never used
| }, | ||
|
|
||
| _isParameterMissing: function (v, vname, vtype) { | ||
| if (typeof v === "undefined") { |
There was a problem hiding this comment.
Please use single quotes for string
| return undefined; | ||
| } | ||
| return { | ||
| "processor": parser, |
There was a problem hiding this comment.
Please use single quotes for string
| } | ||
| return { | ||
| "processor": parser, | ||
| "ext": ext |
There was a problem hiding this comment.
Please use single quotes for string
|
Hi. Thanks for the review. I fixed eslint errors you listed, plus |
|
Wonderfull ! Thank you again for this great contribution ! |
Added:
FileLoader.loadData(data, name, ext)where:datais the input data to load (a GeoJson, GPX or KML)nameis the name of the data (file name, URL, etc)extis an optional parameter to set data type in order to force parser type'data:error'if load/loadData mandatory parameters are missingfile.testingto true to skip reader load)loadDataSample usage (with JQuery):