[Map-Viewer] Added layer from a GeoJSON file #777
Conversation
|
Affected libs:
|
| this.loading = true | ||
| try { | ||
| if (!this.isFileFormatValid(file)) { | ||
| this.errorMessage = 'Invalid file format' |
There was a problem hiding this comment.
Maybe we can save the message in a constant outside this class to reuse it again, e.g. in line 52.
It's better to have a single source of truth. The sentence might change in the future and it could be messy to edit the message in several places.
There was a problem hiding this comment.
I have refactored the code to define the message as a constant for reuse.
| } | ||
|
|
||
| private isFileFormatValid(file: File): boolean { | ||
| const fileExtension = file.name.split('.').pop() |
There was a problem hiding this comment.
This looks quite similar to the logic in line 46.
Maybe you refactor this into a function and reuse it?
There was a problem hiding this comment.
I have refactored it into a function.
| } | ||
| this.mapFacade.addLayer({ ...layerToAdd, title: title }) | ||
| this.successMessage = 'File successfully added to map' | ||
| setTimeout(() => { |
There was a problem hiding this comment.
You are already using a setTimeout function in the displayError function. Maybe you could create a displayMessage function instead and use it for the error and success case but with a different message. WDYT?
There was a problem hiding this comment.
Thanks for pointing it out. I have implemented this change. @Angi-Kinas
| }) | ||
|
|
||
| describe('handleFileChange', () => { | ||
| describe('should set error message if file is not selected', () => { |
There was a problem hiding this comment.
Just a remark in terms of UT naming :
Usually, we try to reserve the expectations, such as should do... to it blocks.
For describe blocks, we try to have either a condition (such as if file is not selected), or the name of the section we're testing (eg : #errorMessage,like you did line 40 for instance).
So for instance in this case, IMO, the name of the describe block should be if file is not selected (and the naming of the following it case is alright since it starts with should).
This remark is true for the next few tests in this file 🙂
There was a problem hiding this comment.
@cmoinier Thanks for the note on UT naming. I have revised the describe blocks for clarity and consistency as suggested.
| this.errorMessage = message | ||
| } | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
@jahow do we have a notification système ? Could be another task for @ronitjadhav what do you guys think ?
There was a problem hiding this comment.
There's no global notification system and we might not need one, since for now all errors are "situational". Let's keep it like that for now.
| } | ||
|
|
||
| const fileExtension = this.getFileExtension(file) | ||
| switch (fileExtension) { |
There was a problem hiding this comment.
One improvement here would be to use DuckDB spatial WASM, to be able to load any kind of spatial dataset.
| } | ||
|
|
||
| setTimeout(() => { | ||
| if (type === 'success') { |
There was a problem hiding this comment.
This condition might not be necessary, you can just set the values to null (as discussed).
ronitjadhav
force-pushed
the
mv-add-from-geojson
branch
from
7a58228
to
ffc87e2
Compare
This pull request adds the feature that allows users to add a layer from a GeoJSON file to their map.
add-layer-from-filecomponent insrc/app/components/add-layer-from-file/add-layer-from-file.component.tsTask List:
Screenshots:
#756