added a save button to export JSON saved state of images #1161

leilayesufu · 2022-10-14T22:30:33Z

i created a button in the archive.html, then i used font awesome to add a save icon in it and added it to the sidebar, then i created an array to push every image in, i then put added all the image to a collection then i ran the generateExportJson() on the collection

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!

gitpod-io · 2022-10-14T22:30:40Z

leilayesufu · 2022-10-14T22:33:04Z

@jywarren Kindly review and leave corrections, thank you

jywarren

This is a great start, thank you! I made a bunch of suggestions. Thanks a lot!

examples/archive.html

jywarren · 2022-10-15T17:47:50Z

examples/archive.html

                placeButton.classList.add('btn', 'btn-sm', 'btn-outline-secondary', 'place-button');
                placeButton.innerHTML = 'Place on map';
-
+                let imageGroup = L.distortableCollection().addTo(map)


Here, I'm not sure if we have to add it to the map - i think images will be added to the map when the user clicks "Place", right? Maybe we leave out addTo(map)?

I’ve also made this change

jywarren · 2022-10-15T17:49:16Z

examples/archive.html

                imageRow.classList.add('d-flex', 'justify-content-between', 'align-items-center', 'mb-4', 'pe-5');
                imageRow.append(image, placeButton);
-
+                imageArray.push[image]


Should we do this only when the user presses the Place button? Maybe in the click handler down on line 141? https://github.com/publiclab/Leaflet.DistortableImage/pull/1161/files#diff-4e935909ea43d3a9c21496afdeae5865a74c6d7ee77640cf467b6e3911b6c63dL141

jywarren · 2022-10-15T17:50:01Z

examples/archive.html

                imageContainer.appendChild(imageRow);
                imageCount++;
+
+                for(let i=0; i<imageArray.length;i++){


And likewise, here maybe we don't need to iterate with a for loop - we can just run imageGroup.addLayer(image[i]) down on line 141 when the user clicks "Place" -- does that make sense?

jywarren · 2022-10-15T17:51:23Z

examples/archive.html

@@ -65,13 +67,18 @@ <h3 id="offcanvasRightLabel">Images</h3>
  </body>

  <script>
-    let map;
+    let map = L.map('map').setView([51.505, -0.09], 13);


Hi, I think this may be redundant with line 84, no?

jywarren · 2022-10-15T17:52:21Z

examples/archive.html

@@ -11,6 +11,8 @@
    <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bootstrap@5.0.2/dist/css/bootstrap.min.css" integrity="sha384-EVSTQN3/azprG1Anm3QDgpJLIm9Nao0Yz1ztcQTwFspd3yD65VohhpuuCOmLASjC" crossorigin="anonymous">
    <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">

+    <script src="../src/DistortableCollection.js"></script>


Here, I am not sure we need this or the next line in this PR, maybe these are left over from another pull request? Yes - it looks like you are making a PR from your main branch, so this can happen. It's OK as long as you remove these lines now, but in the future you can avoid this by working in a named branch for each new feature, instead of in your main branch!

Hii @jywarren
I’ve made all the required changes and the script I added was for the generateExportJson() so the function could be passed down
And yes yes, next time I’d work on a different branch
Thank youu

Aha, so what will happen is if you compile the code (I believe it's grunt build?) It will automatically include all files in the src directory into the files in /dist/, so you don't need to separately do this!

Ah, actually it's "npm run build" as shown here: https://github.com/publiclab/Leaflet.DistortableImage/blob/main/CONTRIBUTING.md#setup

Ohh
Alright
I didn’t know that
Thank youu
Let me make the correction

Done
Thank youu

The script tag below it is for the font awesome icon on the button.
The save icon

jywarren

I made a bunch of suggested changes; see what you think! If they make sense, you can commit them, or ask questions!

I also want to admit that I'm not 100% sure this will work. We'll have to try it out to see. It could be that we actually have to wait until the last moment to add them all to the collection, instead of adding them as we go? But let's try getting this attempt running first and we'll be able to see when we try it out.

Thanks!

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

examples/archive.html

@@ -146,6 +156,8 @@ <h3 id="offcanvasRightLabel">Images</h3>
        let imageURL = event.target.previousElementSibling.src;
        let image = L.distortableImageOverlay(imageURL);
        map.imgGroup.addLayer(image);
+        placedImagesArray.push[image]
+        imageGroup.addLayer(image[i])


here, i think we can just say image and not image[i] anymore, since we aren't in a for loop anymore.

Suggested change

imageGroup.addLayer(image[i])

placedImagesCollection.addLayer(image)

jywarren · 2022-10-18T01:28:48Z

examples/archive.html

@@ -137,6 +145,8 @@ <h3 id="offcanvasRightLabel">Images</h3>
        });
    });

+    saveButton.addEventListener('click',generateExportJson(imageGroup))


Very close here! I think we will want to run it like this:

Suggested change

saveButton.addEventListener('click',generateExportJson(imageGroup))

saveButton.addEventListener('click', function() {

alert(placedImagesCollection.generateExportJson()); // we'll do something better than alert in the next PR

}

jywarren · 2022-10-18T01:30:05Z

examples/archive.html

@@ -72,6 +73,12 @@ <h3 id="offcanvasRightLabel">Images</h3>
    let input = document.getElementById('input');
    let responseText = document.getElementById('response');
    let imageContainer = document.getElementById('imgContainer');
+    let imageGroup = L.distortableCollection().addTo(map)


Let's rename this "placedImagesCollection" and remove the addTo(map)!

Suggested change

let imageGroup = L.distortableCollection().addTo(map)

let placedImagesCollection = L.distortableCollection()

jywarren · 2022-10-18T01:30:39Z

examples/archive.html

@@ -114,14 +121,15 @@ <h3 id="offcanvasRightLabel">Images</h3>

                placeButton.classList.add('btn', 'btn-sm', 'btn-outline-secondary', 'place-button');
                placeButton.innerHTML = 'Place on map';
-
+                let imageGroup = L.distortableCollection()


Here we shouldn't have to declare this a second time. Let's remove it!

Suggested change

let imageGroup = L.distortableCollection()

leilayesufu · 2022-10-18T05:16:56Z

@jywarren
Hope you’re having a great day, I have made the corrections and it all made sense to me, I do believe that this PR is on the right path and if it doesn’t work, it’s minor errors we can fix!
Thank youuu

jywarren · 2022-10-19T13:37:52Z

This looks great! Thank you for all the changes. Are you able to test it out and upload a video? I'm trying to right now but my connection isn't very good.

examples/archive.html

Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>

jywarren · 2022-10-19T14:05:27Z

To test it, I navigate to /examples/archive.html from the root URL it's already at.

leilayesufu · 2022-10-19T14:07:00Z

Thank youu

jywarren · 2022-10-19T14:09:41Z

Just repeating here:

OK, so what I found was that, for some reason, this doesn't work... the images array is empty. But! I also found that we actually already have an distortableImageCollection in this page -- on line 96!

So on this line, we can do:

      alert(JSON.stringify(map.imgGroup.generateExportJson())); // we'll do something better than alert in the next PR

And we can also remove the lines on 76 and 77, since we are just going to use the Collection that's already defined in the page. What do you think?

jywarren · 2022-10-19T14:15:44Z

Hmm. I can't seem to get this to work... it did work for a moment, but I'm not sure what I had changed.

Aha! Since there's a check on line 197:

Leaflet.DistortableImage/src/DistortableCollection.js

Lines 192 to 197 in c80a7c6

    
           generateExportJson() { 
        
             const json = {}; 
        
             json.images = []; 
        
             this.eachLayer(function(layer) { 
        
               if (this.isCollected(layer)) {

...for whether the images have been selected, we actually have to go through and select the images. Hmm, how do we do that?

jywarren · 2022-10-19T14:22:12Z

Can we just add the "selected" class? We need to research how this is supposed to be done.

…

On Wed, Oct 19, 2022, 10:07 AM Leila Yesufu ***@***.***> wrote: Thank youu — Reply to this email directly, view it on GitHub <#1161 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J45BR66SJJA3S4UNODWD76A7ANCNFSM6AAAAAARFT547Y> . You are receiving this because you were mentioned.Message ID: ***@***.***>

leilayesufu · 2022-10-20T19:18:12Z

Hii @jywarren
I’m sorry I haven’t been active the past day
I don’t think we should use the collection that’s already defined on the page in our click listener cause we have to addlayer(image) on it

leilayesufu · 2022-10-20T19:20:23Z

Also about the line 197
I found that the is a function
So I just thought how about we just pass that function into the generateExportJson function

jywarren · 2022-10-30T00:23:22Z

So I just thought how about we just pass that function into the generateExportJson function

Hi @leilayesufu i wonder... what if we make a parameter for generateExportJson() that is something like onlySelected and we only filter for isCollected if that is true, and we default it to true? That way we can use generateExportJson for this use case too, but just set onlySelected to false so we just get everything. What do you think?

leilayesufu · 2022-10-30T01:23:34Z

Hii @jywarren
I was wondering how you were cause I hadn’t heard a review from you a while but I figured you were probably busy with other pull requests
I’d get working on your suggestions and update you

jywarren · 2022-10-30T18:04:47Z

Thank you! Yes i had some big work commitments the past week but am catching up now!

jywarren · 2022-11-26T16:59:41Z

Just noting that I've linked here from #1225 (comment) where there was a parallel effort also requiring this modified generateExportJson(isCollected = true)

jywarren · 2023-02-10T05:43:43Z

Solved in #1237, thank you!!!

added a button to export JSON state of images

6133ede

jywarren requested changes Oct 15, 2022

View reviewed changes

leilayesufu and others added 4 commits October 15, 2022 20:07

made requested changes

5a2fe3c

Update archive.html

0f8ed80

Merge branch 'main' into main

5bc5eae

Merge branch 'main' into main

aef26dd

jywarren requested changes Oct 18, 2022

View reviewed changes

leilayesufu added 2 commits October 18, 2022 06:09

Update archive.html

6f9ec14

Update archive.html

cfaba7c

Merge branch 'main' into main

4465106

Merge branch 'main' into main

7306e80

jywarren reviewed Oct 19, 2022

View reviewed changes

examples/archive.html Outdated Show resolved Hide resolved

Update examples/archive.html

2985061

Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>

leilayesufu added 3 commits October 20, 2022 18:03

corrected an error with syntax

d0d02d6

update archive.html

c1822e5

update archive.html

994693c

jywarren mentioned this pull request Oct 30, 2022

save map coordinates in local storage #1225

Open

5 tasks

jywarren mentioned this pull request Jan 17, 2023

Download json file of saved positions #1237

Merged

5 tasks

jywarren closed this Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added a save button to export JSON saved state of images #1161

added a save button to export JSON saved state of images #1161

leilayesufu commented Oct 14, 2022

gitpod-io bot commented Oct 14, 2022

leilayesufu commented Oct 14, 2022

jywarren left a comment

jywarren Oct 15, 2022

leilayesufu Oct 15, 2022

jywarren Oct 15, 2022

jywarren Oct 15, 2022

jywarren Oct 15, 2022

jywarren Oct 15, 2022

leilayesufu Oct 15, 2022

jywarren Oct 15, 2022

jywarren Oct 15, 2022

leilayesufu Oct 15, 2022

leilayesufu Oct 15, 2022

leilayesufu Oct 15, 2022

jywarren left a comment

jywarren Oct 18, 2022

jywarren Oct 18, 2022

jywarren Oct 18, 2022

jywarren Oct 18, 2022

leilayesufu commented Oct 18, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

leilayesufu commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022 via email

leilayesufu commented Oct 20, 2022

leilayesufu commented Oct 20, 2022

jywarren commented Oct 30, 2022

leilayesufu commented Oct 30, 2022

jywarren commented Oct 30, 2022

jywarren commented Nov 26, 2022

jywarren commented Feb 10, 2023

	imageGroup.addLayer(image[i])
	placedImagesCollection.addLayer(image)

-    saveButton.addEventListener('click',generateExportJson(imageGroup))
+    saveButton.addEventListener('click', function() {
+      alert(placedImagesCollection.generateExportJson()); // we'll do something better than alert in the next PR
+    }

	let imageGroup = L.distortableCollection().addTo(map)
	let placedImagesCollection = L.distortableCollection()

added a save button to export JSON saved state of images #1161

added a save button to export JSON saved state of images #1161

Conversation

leilayesufu commented Oct 14, 2022

gitpod-io bot commented Oct 14, 2022

leilayesufu commented Oct 14, 2022

jywarren left a 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

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 left a 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

leilayesufu commented Oct 18, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

leilayesufu commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022

jywarren commented Oct 19, 2022 via email

leilayesufu commented Oct 20, 2022

leilayesufu commented Oct 20, 2022

jywarren commented Oct 30, 2022

leilayesufu commented Oct 30, 2022

jywarren commented Oct 30, 2022

jywarren commented Nov 26, 2022

jywarren commented Feb 10, 2023