Added spining icon for `place on map` button #1169

RealRichi3 · 2022-10-15T19:40:56Z

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!

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
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updates
@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!

…bleImage

gitpod-io · 2022-10-15T19:41:02Z

RealRichi3 · 2022-10-15T19:43:23Z

@sainamercy your logic worked perfectly

RealRichi3 · 2022-10-15T19:45:17Z

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

This is how it is currently

RealRichi3 · 2022-10-15T20:25:14Z

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

jywarren · 2022-10-15T21:50:07Z

examples/archive.html

@@ -108,6 +108,26 @@ <h3 id="offcanvasRightLabel">Images</h3>
 placeButton.classList.add('btn', 'btn-sm', 'btn-outline-secondary', 'place-button');
 placeButton.innerHTML = 'Place on map';


This looks super! One small spacing suggestion:

Suggested change

placeButton.innerHTML = 'Place on map';

placeButton.innerHTML = 'Place on map ';

Ohh, alright, I'll make the necessary change

@jywarren i have added the white space. Looks good now?

sainamercy · 2022-10-16T03:45:40Z

examples/archive.html

+ placeButton.addEventListener('click', (event) => {
+ if(!placeButton.getElementsByClassName('fa-spinner')[0]){
+ const spinner = document.createElement('i')
+ spinner.setAttribute('class', "fa fa-spinner fa-spin")
+ spinner.setAttribute('font-family', 'font awesome')
+
+ if (event.target.classList.contains('place-button')){


@RealRichi3 @jywarren, since the click event has been placed directly on the placeButton, could we get rid of the check on line 117?

you mean we should add the spinner without the check?

Yes. As opposed to the earlier version document.addEventListener('click', (event)=>{... , there is no event delegation in this. The event has been placed directly to the target.

@RealRichi3 We have a repetition of code. check lines 157 to 161, that is where I had earlier suggested the spinner go.

Deleting lines 157-161 should clean it up right?

@RealRichi3 it would. However, performance-wise, it won't be effective since we are creating an event on every button. I still prefer the one I suggested. This is what my solution looks like:

document.addEventListener("click", (event) => { if (event.target.classList.contains("place-button")) { const targetPlaceButton = event.target; const spinner = document.createElement('i'); spinner.classList.add('fa', 'fa-spinner', 'fa-spin'); targetPlaceButton.append(spinner); let imageURL = event.target.previousElementSibling.src; let image = L.distortableImageOverlay(imageURL); map.imgGroup.addLayer(image); image.addEventListener("load", () => { spinner.remove() }); } });

Thanks @sainamercy, i added an extra check incase a user clicks the button multiple times, this will add more than one spinner icon. But it's all good now

I think you were getting the error coz it's wrapped around the forEach block. Try outside the block, maybe where it was before, then see if there's a difference.

Ohh, i understand now, but i think it looks good now. @jywarren please review

…bleImage into feat/spining_icon

…flet.DistortableImage into feat/spining_icon

jywarren · 2022-10-18T01:43:07Z

examples/archive.html

+ if(event.target.classList.contains('place-button')){
+ const targetPlaceButton = event.target;
+
+ if(!targetPlaceButton.getElementsByClassName('fa-spinner')[0]){


Hi @RealRichi3 I see you're getting this to run only if it doesn't have a spinner, so it's run only once each time the forEach is run. But I see what @sainamercy is saying that this code was originally written to run just once, outside the forEach, and so anything outside this conditional will run many times. It just seems to me we might as well leave the addEventListener("click") where it originally was at the bottom, OR to change it so it's listening only for a click on the button itself, like:

placeButton.addEventListener('click', (event) => {

...instead of on the whole document (it used to be that way because we handled all clicks in a single listener instead of setting one up each time a button is created). Does that make sense? You could do either one but here we are kind of mixing the two.

Otherwise this looks great. If you can try solving this and uploading a screen recording of it working, that would be super! Thanks both of you!

Thanks @jywarren, so you mean i should change the event listener to target the button itself instead of the document

jywarren · 2022-10-18T11:44:15Z

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 <#1169 (comment)> : > @@ -113,7 +113,31 @@ <h3 id="offcanvasRightLabel">Images</h3> 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 <#1169 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J24KKN7KUOTPGYKN7LWDZ3PBANCNFSM6AAAAAARGBDMZQ> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

RealRichi3 · 2022-10-18T16:19:43Z

@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 · 2022-10-18T16:36:58Z

@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?

…flet.DistortableImage into feat/spining_icon

jywarren · 2022-10-18T17:24:56Z

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 <#1169 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J2RIWTWVOVHTWQSQ4TWD3G3NANCNFSM6AAAAAARGBDMZQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

RealRichi3 · 2022-10-18T18:19:38Z

Alright @jywarren

jywarren · 2022-10-19T13:00:18Z

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 · 2022-10-19T13:19:29Z

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

jywarren · 2022-10-19T13:27:36Z

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!

jywarren

Nicely done, both of you!

RealRichi3 · 2022-10-19T18:37:39Z

Thanks @jywarren and @sainamercy

sainamercy · 2022-10-20T05:33:32Z

@RealRichi3 @jywarren Thanks too

RealRichi3 and others added 7 commits October 11, 2022 00:55

Replaced var keywords with let or const - /src/DistortableCollection.js

7fc2591

Merge branch 'main' into main

c23fd7b

add: included font awesome v4 to head - archive.html

2db0f8b

Merge branch 'main' of https://github.com/RealRichi3/Leaflet.Distorta…

9af2879

…bleImage

add: included font awesome v4 to head - archive.html

6d74655

Merge branch 'publiclab:main' into main

967f018

add: spinning icon for place_on_map button - archive.html

25aa366

RealRichi3 changed the title ~~spining icon~~ Added spining icon for place on map button Oct 15, 2022

jywarren reviewed Oct 15, 2022

View reviewed changes

add white space to place_button text

c49ef84

sainamercy reviewed Oct 16, 2022

View reviewed changes

RealRichi3 and others added 5 commits October 17, 2022 01:46

removed duplicate code for place_button eventlistener - archive.htl

d296436

Merge branch 'main' of https://github.com/RealRichi3/Leaflet.Distorta…

88a51f0

…bleImage into feat/spining_icon

Merge branch 'feat/spining_icon' of https://github.com/RealRichi3/Lea…

0198e79

…flet.DistortableImage into feat/spining_icon

moved place-button click eventlistener to document - archive.html

93f633b

Merge branch 'main' into feat/spining_icon

837593d

jywarren requested changes Oct 18, 2022

View reviewed changes

RealRichi3 added 2 commits October 18, 2022 17:40

change eventlistener target to place_button - archive.html

35d6765

Merge branch 'feat/spining_icon' of https://github.com/RealRichi3/Lea…

f7f3275

…flet.DistortableImage into feat/spining_icon

RealRichi3 requested a review from jywarren October 19, 2022 09:25

Merge branch 'main' into feat/spining_icon

474f0c8

jywarren approved these changes Oct 19, 2022

View reviewed changes

jywarren merged commit 1c5c6e7 into publiclab:main Oct 19, 2022

7malikk mentioned this pull request Dec 7, 2022

MapKnitter Lite: fetch images from Internet Archive #998

Open

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added spining icon for `place on map` button #1169

Added spining icon for `place on map` button #1169

RealRichi3 commented Oct 15, 2022 •

edited

Loading

gitpod-io bot commented Oct 15, 2022

RealRichi3 commented Oct 15, 2022

RealRichi3 commented Oct 15, 2022 •

edited

Loading

RealRichi3 commented Oct 15, 2022

jywarren Oct 15, 2022

RealRichi3 Oct 15, 2022

RealRichi3 Oct 16, 2022

sainamercy Oct 16, 2022

RealRichi3 Oct 16, 2022

sainamercy Oct 16, 2022

sainamercy Oct 16, 2022

RealRichi3 Oct 17, 2022

sainamercy Oct 17, 2022

RealRichi3 Oct 17, 2022

sainamercy Oct 17, 2022 •

edited

Loading

RealRichi3 Oct 17, 2022

jywarren Oct 18, 2022

RealRichi3 Oct 18, 2022

jywarren commented Oct 18, 2022 via email

RealRichi3 commented Oct 18, 2022 •

edited

Loading

RealRichi3 commented Oct 18, 2022 •

edited

Loading

jywarren commented Oct 18, 2022 via email

RealRichi3 commented Oct 18, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren left a comment

RealRichi3 commented Oct 19, 2022

sainamercy commented Oct 20, 2022

		@@ -108,6 +108,26 @@ <h3 id="offcanvasRightLabel">Images</h3>
		placeButton.classList.add('btn', 'btn-sm', 'btn-outline-secondary', 'place-button');
		placeButton.innerHTML = 'Place on map';

	placeButton.innerHTML = 'Place on map';
	placeButton.innerHTML = 'Place on map ';

Added spining icon for place on map button #1169

Added spining icon for place on map button #1169

Conversation

RealRichi3 commented Oct 15, 2022 • edited Loading

gitpod-io bot commented Oct 15, 2022

RealRichi3 commented Oct 15, 2022

RealRichi3 commented Oct 15, 2022 • edited Loading

RealRichi3 commented Oct 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sainamercy Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Oct 18, 2022 via email

RealRichi3 commented Oct 18, 2022 • edited Loading

RealRichi3 commented Oct 18, 2022 • edited Loading

jywarren commented Oct 18, 2022 via email

RealRichi3 commented Oct 18, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren left a comment

Choose a reason for hiding this comment

RealRichi3 commented Oct 19, 2022

sainamercy commented Oct 20, 2022

Added spining icon for `place on map` button #1169

Added spining icon for `place on map` button #1169

RealRichi3 commented Oct 15, 2022 •

edited

Loading

RealRichi3 commented Oct 15, 2022 •

edited

Loading

sainamercy Oct 17, 2022 •

edited

Loading

RealRichi3 commented Oct 18, 2022 •

edited

Loading

RealRichi3 commented Oct 18, 2022 •

edited

Loading