Esri / esri-leaflet-vector

Display ArcGIS Online vector basemaps w/ Esri Leaflet
Apache License 2.0
56 stars 57 forks source link

Issue #158 #202

Closed sdthaker closed 11 months ago

sdthaker commented 1 year ago

Changes Made:

Testing

Issue

Additional Notes

Please let me know if there are any other changes that are to be made as part of the PR. I might've missed something considering I'm quite new to this repo and open-source development in general. Thanks!

sdthaker commented 1 year ago

So should I revert back the code that I changed for all the test cases in spec/VectorTileLayerSpec.js? Thanks

patrickarlt commented 1 year ago

@sdthaker I would probably start by reverting all changes you have made so far (sorry) and adding a check on these lines https://github.com/Esri/esri-leaflet-vector/blob/master/src/VectorTileLayer.js#L62-L67 when there is an error loading the style add the error messages there. You'll need to test lots of different scenarios with an invalid token (just any random string) to make sure you catch all the different error messages since each of these scenarios will produce different error messages:

sdthaker commented 1 year ago

After adding a console.log for the error variable, above line 62, building the static HTML files and opening each of them on the browser, I always see the error being printed as either undefined or null on the browser console. Am I testing this right?

For some of the files, the error is not printed at all since I believe the execution of the script is halted somewhere before the code execution reaches L62-L67 since it shows some other error regarding this #102

sdthaker commented 1 year ago

I've added a console.log() to show the error to the user. I tested the first 3 bullet points, by manually editing examples/custom-vtl-dev.html script tag and it logged the error An error occurred: Invalid token. I was unsure about how to test the last 2 bullet points. Please let me know if the commit I made makes sense and if there is anything else to be added/removed as part of this PR.

I apologize, I'm quite new to this project so everything is haywire for me right now, still trying to make sense of what's the purpose of this project, what the classes in the src directory do, how everything is connected together etc, etc. Thanks! @patrickarlt

sdthaker commented 1 year ago

Hi, please provide an update on this PR, whether it needs anything else to be added or if it's good to be merged! Thanks!