Skip to content

Conversation

arjxn-py
Copy link
Member

Should fix #140 & #117

jgis-shp.1.1.mov

Copy link
Contributor

Binder 👈 Launch a Binder on branch arjxn-py/jupytergis/fix-shapefile

@brichet brichet added the bug Something isn't working label Sep 16, 2024
Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks @arjxn-py, this looks great.

It can be in follow up PR:

  • can we add tests on it to prevent it from being broken
  • looks like changing the color only affect the border of the shapes. Would it be easy to propagate it to the background also ?

@arjxn-py
Copy link
Member Author

Hi @brichet, thanks for the review :)

can we add tests on it to prevent it from being broken

Sure, I'll be happy to add those, haven't added one to JGIS before so I'd once have to give a look to other tests and PR how they do it.

looks like changing the color only affect the border of the shapes. Would it be easy to propagate it to the background also ?

Yeah, I notice that too. I'll be happy to look into that as well. On a side note I'll also see if the same is happening for GeoJSON layer too as in the case of ShapeFile too, it is first being converted to GeoJson and then being overlayed on the map. But at the moment i'm quite unsure about the complexity of the same hence if it's easy I'll be sure to fix it directly but in case I couldn't manage to I'll be happy to open an issue for the same to keep track.

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks @arjxn-py !

@martinRenou martinRenou merged commit 7cf2425 into geojupyter:main Sep 16, 2024
9 of 10 checks passed
@brichet
Copy link
Collaborator

brichet commented Sep 16, 2024

But at the moment i'm quite unsure about the complexity of the same hence if it's easy I'll be sure to fix it directly but in case I couldn't manage to I'll be happy to open an issue for the same to keep track.

I was looking at it and you're right, the issue is the same for the GeoJSON layer.
It has to be fixed for the VectorLayer, I think probably at

style: currentFeature =>

@arjxn-py
Copy link
Member Author

But at the moment i'm quite unsure about the complexity of the same hence if it's easy I'll be sure to fix it directly but in case I couldn't manage to I'll be happy to open an issue for the same to keep track.

I was looking at it and you're right, the issue is the same for the GeoJSON layer. It has to be fixed for the VectorLayer, I think probably at

style: currentFeature =>

Thanks for looking into it already 🚀, this is helpful. I'll be happy to make a quick try on it to see if it can be fixed easily.

@arjxn-py arjxn-py deleted the fix-shapefile branch September 16, 2024 12:17
@arjxn-py arjxn-py mentioned this pull request Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot load ShapeFile
3 participants