search

publiclab / Leaflet.DistortableImage

A Leaflet extension to distort or "rubber sheet" images
https://publiclab.github.io/Leaflet.DistortableImage/examples/
BSD 2-Clause "Simplified" License
265 stars 284 forks source link

Reconstruct Map from URL #1339

Closed segun-codes closed 1 year ago

segun-codes commented 1 year ago

Fixes #1333 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

segun-codes commented 1 year ago

@jywarren, just for you to see progress made so far and probably offer some advice, work not fully ready yet.

segun-codes commented 1 year ago

@jywarren, this PR is now ready for your further review. You may refer to illustrations 1 & 2 below and then offer your thoughts. Many thanks!

Illustration 1: URLs used for testing:

  1. http://localhost:8081/examples/shareable.html?json=https://archive.org/download/segeotest/segeotest.json
  2. http://localhost:8081/examples/shareable.html?json=https://archive.org/download/segeotest2/segeotest2.json
  3. http://localhost:8081/examples/shareable.html?json=https://archive.org/download/segeotest3/segeotest3.json
  4. http://localhost:8081/examples/shareable.html?json=https://archive.org/download/segeotest4/segeotest4.json

Illustration 2: Illustration

segun-codes commented 1 year ago

Hi @segun-codes this is exciting! It's looking really good. I made some suggestions, first to separate out fetching from recreating the map, and second, to move all URL-related code outside the core library. What do you think?

Okay, so this is also a good idea.

You know I figure if we agree that the sharing feature (generating JSON and reconstructing maps from the JSON) is core, then it should be natively supported and shipped in the library. This feature is so basic nowadays that the library users will naturally expect to expend quite minimal effort to wire it up using proper documentation. Externalizing the codes into examples folder means more work for users but then we can still follow this route if it's relatively more beneficial.

segun-codes commented 1 year ago

Hi @jywarren, the code has been restructured (and some renaming done) as per your advice, Could you kindly take a look, many thanks!

jywarren commented 1 year ago

Hi @segun-codes --

You know I figure if we agree that the sharing feature (generating JSON and reconstructing maps from the JSON) is core, then it should be natively supported and shipped in the library. This feature is so basic nowadays that the library users will naturally expect to expend quite minimal effort to wire it up using proper documentation. Externalizing the codes into examples folder means more work for users but then we can still follow this route if it's relatively more beneficial.

I hear your point for sure. However, I want to suggest that Leaflet.DistortableImage is designed as a library, not an application. This is the slightly odd thing about MapKnitter Lite, that it's an application we are developing inside the library repository. We could at some point decide that we want a separate, stand-alone application repository, like https://github.com/publiclab/mapknitter.js, or https://github.com/publiclab/mapknitter-lite. But for now, we have to draw those distinctions ourselves conceptually and in where we place the code.

For MapKnitter Lite it definitely makes sense to include the URL code. But for other applications using the Leaflet.DistortableImage (LDI) library, they may have their own logic, or their own parameters or routing. Many web applications use URL parameters, and they may do so in different ways. I think it's reasonable to ask LDI to read a remote JSON file. But to read and write the current URL could easily conflict with other applications' logic, and create incompatibilities. That's how I'm thinking about it, I hope it makes sense. I think we wouldn't be in this position if we'd decided earlier to clearly break out MK Lite into its own application. But if we keep our code well organized, we can choose to do that in the future. What do you think?

segun-codes commented 1 year ago

Hi @segun-codes --

You know I figure if we agree that the sharing feature (generating JSON and reconstructing maps from the JSON) is core, then it should be natively supported and shipped in the library. This feature is so basic nowadays that the library users will naturally expect to expend quite minimal effort to wire it up using proper documentation. Externalizing the codes into examples folder means more work for users but then we can still follow this route if it's relatively more beneficial.

I hear your point for sure. However, I want to suggest that Leaflet.DistortableImage is designed as a library, not an application. This is the slightly odd thing about MapKnitter Lite, that it's an application we are developing inside the library repository. We could at some point decide that we want a separate, stand-alone application repository, like https://github.com/publiclab/mapknitter.js, or https://github.com/publiclab/mapknitter-lite. But for now, we have to draw those distinctions ourselves conceptually and in where we place the code.

For MapKnitter Lite it definitely makes sense to include the URL code. But for other applications using the Leaflet.DistortableImage (LDI) library, they may have their own logic, or their own parameters or routing. Many web applications use URL parameters, and they may do so in different ways. I think it's reasonable to ask LDI to read a remote JSON file. But to read and write the current URL could easily conflict with other applications' logic, and create incompatibilities. That's how I'm thinking about it, I hope it makes sense. I think we wouldn't be in this position if we'd decided earlier to clearly break out MK Lite into its own application. But if we keep our code well organized, we can choose to do that in the future. What do you think?

Okay, I get your point and it's fine. Many thanks!

segun-codes commented 1 year ago

This looks super. Really nicely formatted and documented code. Thank you!

I think you had mentioned that shareable.js was a temporary solution, and I wonder if it's worth trying to re-integrate your new code with archive.js now. Otherwise we'll be creating a new file just to remove it again later. But it's your choice!

I did mark one function that I think could go in the core library, as it wouldn't interrupt other application logic. But again its your choice!

I am happy to fix this too. I will move everything to archive.js.

jywarren commented 1 year ago

This is going to be super!

I wanted to ask @segun-codes and @7malikk to think on this, maybe on Monday - and we can discuss at our Tuesday meeting. How might we think about a follow-up PR, which could abstract/modularize a lot of this code into a separate file? What are some ideas? We could brainstorm in a new issue maybe?

segun-codes commented 1 year ago

This is going to be super!

I wanted to ask @segun-codes and @7malikk to think on this, maybe on Monday - and we can discuss at our Tuesday meeting. How might we think about a follow-up PR, which could abstract/modularize a lot of this code into a separate file? What are some ideas? We could brainstorm in a new issue maybe?

Okay, that's fine.

segun-codes commented 1 year ago

Hi @jywarren, renaming effected. Do you want to consider for merging. Many thanks!

jywarren commented 1 year ago

Amazing!!! Congrats, fantastic work!

segun-codes commented 1 year ago

Amazing!!! Congrats, fantastic work!

Many thanks!

7malikk commented 1 year ago

Grat work on @segun-codes!! 🎉

segun-codes commented 1 year ago

Grat work on @segun-codes!! 🎉

Many thanks!!!