Added spining icon for `place on map` button

[RealRichi3]

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

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

• [x] PR is descriptively titled 📑 and links the original issue above 🔗
• [ ] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt test
• [x] code is in uniquely-named feature branch and has no merge conflicts 📁
• [x] screenshots/GIFs are attached 📎 in case of UI updates
• [x] @mention the original creator of the issue in a comment below for help or for a review

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!

[gitpod-io[bot]]

[RealRichi3]

@sainamercy your logic worked perfectly

[RealRichi3]

@sainamercy do you think it'll be better to place the spinning icon outside?,

This is how it is currently

[RealRichi3]

@jywarren please checkout the changes i made, do you think it's better this way?

[jywarren]

I think so, if you're going to run the listener setup for each button anyways, that makes sense and also keeps the button setup code near where the button is created.

On Tue, Oct 18, 2022, 6:26 AM Richie @.***> wrote:

@.**** commented on this pull request.

In examples/archive.html https://github.com/publiclab/Leaflet.DistortableImage/pull/1169#discussion_r998018830 :

@@ -113,7 +113,31 @@

Images

fetchedFrom.appendChild(fetchedFromUrl)

             placeButton.classList.add('btn', 'btn-sm', 'btn-outline-secondary', 'place-button');

•            placeButton.innerHTML = 'Place on map';
  

•            placeButton.innerHTML = 'Place on map ';
  

•            document.addEventListener('click', (event) => {
  

•                if(event.target.classList.contains('place-button')){
  

•                  const targetPlaceButton = event.target;
  

•                if(!targetPlaceButton.getElementsByClassName('fa-spinner')[0]){
  

Thanks @jywarren https://github.com/jywarren, so you mean i should change the event listener to target the button itself instead of the document

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

[RealRichi3]

@jywarren thanks, If that's all i'll make the changes now, also you said something about a video, should i still create the video?

[RealRichi3]

@jywarren I was thinking, what if the image fails to load, the icon will continue spinning. Would it be okay for me to set a delay timer to check for situations like this then replace the spinning icon with an error iccon. What do you think?

[jywarren]

Hi, I think we can address that in a later PR; at least if we leave the spinner running, you'd know which image had failed. Good thinking though!

On Tue, Oct 18, 2022 at 12:37 PM Richie @.***> wrote:

@jywarren https://github.com/jywarren I was thinking, what if the image fails to load, the icon will continue spinning. Would it be okay for me to set a delay timer to check for situations like this. What do you think?

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

[RealRichi3]

Alright @jywarren

[jywarren]

I'm reviewing now but yes, it would be really great to see a video to confirm this works and see how it looks!

[jywarren]

I just started it up and tried it out in GitPod and it looks great! Let's merge!

[jywarren]

Hmm, something odd is happening, it might be because I have a poor internet connection, but although I am seeing the spinner, I'm not seeing the images appearing:

Ah wait! Ok, one loaded, and the spinner disappeared. So this works!

[RealRichi3]

Thanks @jywarren and @sainamercy

[sainamercy]

@RealRichi3 @jywarren Thanks too