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

feat: add Esri ArcGIS Maps SDK adapter #58

Merged

Conversation

Tugark
Copy link
Contributor

@Tugark Tugark commented Jul 6, 2023

Hope you got back well from FOSS4G! ;)

As discussed at FOSS4G, I tried to whip up and Esri ArcGIS Maps SDK adapter.

The functionality is working, so if you want to glance over it, feel free to add suggestions. Couple of things to note:

  • I am using a single GraphicsLayer instead of a GeoJSONLayer. The advantage being that it's less abstraction and we don't have to maintain several layers and split the features accordingly (since the GeoJSONLayer only accepts features of the same type along some other limitations).
    • However, this could be fixed with some more logic, should we require the more powerful GeoJSONLayer (or, FeatureLayer at that)
  • The drawing style is actually not consistent with other libraries in that the width is calculated differently - lines are bolder, points are smaller). I'm not sure if it's your goal to have the "look" identical across libraries; if so, I'd have to dig deeper into the drawing logic of the Esri tool.

I'm opening this PR as a draft because it is still missing tests - I have to work on them as I get the nice old SyntaxError: Unexpected token export and the Esri API is a bit cumbersome to test or mock. I hope I get around it somewhen soon, but I felt that I could already get some feedback on how that works for you.

Once again, thanks for your work so far. I do believe that this library has a lot of potential!

@JamesLMilner
Copy link
Owner

Hey @Tugark - I made it back in one piece thank you. It was great meeting you at FOSS4G this year, thanks for coming to the talk!

This is awesome 🎉 Thanks so much for raising it. I am just on holiday till Monday but will take a look properly when I am back.

I don't think it's necessary to have 100% identical styling, I feel 'best effort' is acceptable as long as they look approximately the same. It can always be a follow up improvement if it proves to be problematic.

@JamesLMilner
Copy link
Owner

JamesLMilner commented Jul 11, 2023

Hey @Tugark Just checked this out - nice work! Code quality is great and all working nicely. I played with the styling and figured out you just need to set any widths to pixels and it matches the styling of the other adpaters. I also needed to set the point size to * 2 (assuming this is the radius in other adapters hence having to double it to get the width).

If you manage to get a chance to write some tests that would be great - current threshold is set to 65% so if we could try and hit that it would be awesome. Let me know if it becomes to arduous and we can look at getting it in without and I can try and follow it up when i get a moment (There is already an open issue for better adapter testing #18)

@Tugark
Copy link
Contributor Author

Tugark commented Jul 16, 2023

Hey @JamesLMilner Thanks for the review! I just added your suggestion and I also added some basic tests. I'll try to add some tests for the render method and the map initializer as well so the test coverage should be similar to the leaflet adapter tests.

I'm currently quite swamped, unfortunately, but I hope I'll be able to squeeze it in after the 23rd.

@JamesLMilner
Copy link
Owner

@Tugark awesome, let me know if I can help at all. No worries on the timings, whenever is convenient for you. It's looking great, excited to get it in!

@Tugark Tugark marked this pull request as ready for review July 25, 2023 19:20
@Tugark Tugark force-pushed the feature/add-arcgis-maps-sdk-adapter branch from 39184fb to 14a3733 Compare July 25, 2023 19:25
@Tugark
Copy link
Contributor Author

Tugark commented Jul 25, 2023

@JamesLMilner Finally got around for adding some more tests - as always, it's a bit messy because it's relatively cumbersome to properly mock the javascript API.

According to the coverage, it looks well:
image

The uncovered lines are callbacks which I'm not sure can be tested. The rest should be good enough for the moment.

Let me know if you want me to clean up the tests even more.

@JamesLMilner
Copy link
Owner

@Tugark amazing work! Thanks so much - I think this is ready to go in now :)

@JamesLMilner JamesLMilner merged commit bf1d333 into JamesLMilner:main Jul 26, 2023
1 check passed
morehawes pushed a commit to OpenGIS/terra-draw that referenced this pull request Dec 5, 2023
The Esri ArcGIS Maps SDK adapter added in JamesLMilner#58 is not exported. This PR adds the export and updated the development/ app.
JamesLMilner pushed a commit that referenced this pull request Dec 5, 2023
The Esri ArcGIS Maps SDK adapter added in #58 is not exported. This PR adds the export and updated the development/ app.

Co-authored-by: Joe Hawes <>
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

2 participants