publiclab / Leaflet.DistortableImage

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

Tooltip Feature for ImageOverlays #1321

Closed segun-codes closed 1 year ago

segun-codes commented 1 year ago

Fixes #1317

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

The GIFs below show the work done so far. However, the tooltip feature is also enabled in files listeners.html, select.html, local.html, and export.html. index.html 01

archive.html 02_2

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

Hi @jywarren, I have made some progress on this PR but getting the tooltip to dynamically position in the right coordinates close to the cursor (for good UX) is what I am still working on. Any thought on this is welcome. Many thanks!

7malikk commented 1 year ago

Great job here @segun-codes

jywarren commented 1 year ago

https://leafletjs.com/reference.html#tooltip

segun-codes commented 1 year ago

Hi @jywarren, I have made some progress on this PR but getting the tooltip to dynamically position in the right coordinates close to the cursor (for good UX) is what I am still working on. Any thought on this is welcome. Many thanks!

@PeculiarE, here's the PR on tooltip we discussed. Many thanks!

segun-codes commented 1 year ago

Hi @jywarren, I was thinking that if okay by you and for the purpose of progress, we could consider this PR done with tooltip displayed at the center of imageOverlay for now. I have reworked the code to work this way on all the .html files in the example folder. This also works correctly in the case where multiple imageOverlays are placed on the tiled layer. If you subscribe to my suggestion, then I will proceed to tidy up the code. See illustration 3 below for more details.

The additional effect of having the tooltip follow the cursor could be captured in another standalone PR. What do you think?

Many thanks!

Illustration 3: Illustration-3

jywarren commented 1 year ago

Hi Segun, My only worry is if we see some situation we haven't anticipated where the tooltip blocks access to something like an image handle or menu. How sure are we that it can't happen? Would it help our confidence to add a way to suppress or close the tooltip? Or switch them off?

If we can address this issue, then I think it's ok to close this. Let me know what you think!

On Fri, Jan 6, 2023, 6:31 PM Segun @.***> wrote:

Hi @jywarren https://github.com/jywarren, I was thinking that if okay by you and for the purpose of progress, we could consider this PR done with tooltip displayed at the center of imageOverlay for now. I have reworked the code to work this way on all the .html files in the example folder. This also works correctly in the case where multiple imageOverlays are placed on the tiled layer. If you subscribe to my suggestion, then I will proceed to tidy up the code. See illustration 3 below for more details.

The additional effect of having the tooltip follow the cursor could be captured in another standalone PR. What do you think?

Illustration 3: [image: Illustration-3] https://user-images.githubusercontent.com/1612359/211116435-c7bb9ba0-911e-41b7-ac37-0ab365c7f4ab.gif

— Reply to this email directly, view it on GitHub https://github.com/publiclab/Leaflet.DistortableImage/pull/1321#issuecomment-1374254077, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J7SCMBVRIX3Z7VLRSTWRCTL5ANCNFSM6AAAAAATMM5IDU . You are receiving this because you were mentioned.Message ID: @.***>

segun-codes commented 1 year ago

Oh! your concern is clear. So, I guess the flexibility of switching off/on tooltip is a good route to take. What do you think about a button on the sidebar to do this switching?

segun-codes commented 1 year ago

Hi Segun, My only worry is if we see some situation we haven't anticipated where the tooltip blocks access to something like an image handle or menu. How sure are we that it can't happen? Would it help our confidence to add a way to suppress or close the tooltip? Or switch them off? If we can address this issue, then I think it's ok to close this. Let me know what you think!

Hi @jywarren, I have re-implemented the tooltip and equip it with the functionality to switch it on and off as you suggested. When switched off, the associated event listeners are turned off completely. Consider the illustration below and advise with your thoughts, many thanks!

Illustration 4 Illustration-4

segun-codes commented 1 year ago

Hi @jywarren, still awaiting your review on the above. Many thanks!

7malikk commented 1 year ago

Great job! 🎉 @segun-codes

segun-codes commented 1 year ago

Hi @jywarren, this PR is now ready for merging. Many thanks!

segun-codes commented 1 year ago

Hi @jywarren, this PR is now ready for merging. Many thanks!

Hi @jywarren, still waiting for you to merge. Many thanks!

jywarren commented 1 year ago

🎉 🎉 🎉 🎉