Multiple Image Select #158
Conversation
|
Wow, this is impressive... how far along is it? I see it clearly relating to publiclab/mapknitter#326 where it'll allow us to initiate an export based on the selected set of images. Great!!! |
|
@jywarren It's just in initial stage to return an array of all images on the screen since that was mentioned in the original issue. I just wrote over the leaflet source code to change the functionality. I will be adding status updates in the PR description box! |
sashadev-sky
force-pushed
the
multiple_image_select
branch
from
86e25aa to
d999f00
Compare
|
@jywarren I think I am stuck here. This is what I have done so far: https://github.com/sashadev-sky/PL-notes/blob/master/giphy.gif Sorry it is blurry! Basically the zoombox makes it so that the images move into a div element inside the overlay pane. From there, any css transformations applied onto the div effects both elements. The transformations are easy to apply in chrome dev tools in the browser, but when I started actually trying to connect the div to the current distortableImage code I realized I am not sure how to do that. My plan was to move images in there and then when the user is done editing multiple images (or selects different ones), move them out. |
|
Let's not worry yet about group transforms, let's do just translation for
starters? Then we could think about rotation and scaling next, and I think
I can share some math for that. But honestly just getting group dragging
(translation in x,y) and some UI to show group selection would be a great
starting point!
…On Mon, Mar 11, 2019, 11:32 PM Sasha Boginsky ***@***.***> wrote:
@jywarren <https://github.com/jywarren> I think I am stuck here. This is
what I have done so far:
https://github.com/sashadev-sky/PL-notes/blob/master/giphy.gif
Sorry it is blurry! Basically the zoombox makes it so that the images move
into a div element inside the overlay pane. From there, any css
transformations applied onto the div effects both elements. The
transformations are easy to apply in chrome dev tools in the browser, but
when I started actually trying to connect the div to the current
distortableImage code I realized I am not sure how to do that. My plan was
to move images in there and then when the user is done editing multiple
images (or selects different ones), move them out.
@rexagod <https://github.com/rexagod>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ0NOaufV2rOogU_ZWtYf70f51dPwks5vVx-RgaJpZM4bR9iJ>
.
|
|
Also, I wonder if they could be added to a Leaflet layer together instead
of a div, what do you think? And then we could relay the translations into
each member of that layer?
…On Mon, Mar 11, 2019, 11:53 PM Jeffrey Warren ***@***.***> wrote:
Let's not worry yet about group transforms, let's do just translation for
starters? Then we could think about rotation and scaling next, and I think
I can share some math for that. But honestly just getting group dragging
(translation in x,y) and some UI to show group selection would be a great
starting point!
On Mon, Mar 11, 2019, 11:32 PM Sasha Boginsky ***@***.***>
wrote:
> @jywarren <https://github.com/jywarren> I think I am stuck here. This is
> what I have done so far:
>
> https://github.com/sashadev-sky/PL-notes/blob/master/giphy.gif
>
> Sorry it is blurry! Basically the zoombox makes it so that the images
> move into a div element inside the overlay pane. From there, any css
> transformations applied onto the div effects both elements. The
> transformations are easy to apply in chrome dev tools in the browser, but
> when I started actually trying to connect the div to the current
> distortableImage code I realized I am not sure how to do that. My plan was
> to move images in there and then when the user is done editing multiple
> images (or selects different ones), move them out.
>
> @rexagod <https://github.com/rexagod>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#158 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AABfJ0NOaufV2rOogU_ZWtYf70f51dPwks5vVx-RgaJpZM4bR9iJ>
> .
>
|
|
@jywarren I was thinking of translation as a subset of transform and that is the part I am stuck on. I definitely think they should be moved into a leaflet layer, I was using the |
|
Hmm, well for starters I think translation is easier because you can just
manipulate the xy of any overlay in leaflet with the leaflet location
management, whereas transform requires the interior css transforms, doesn't
it? Or I forget, do we manage xy with css transform too? Maybe we do...
I wonder if there is an example to look to for leaflet layer and item
grouping behaviors. Maybe someone has implemented a plugin to manage
multiple markers we could look at?
Alternatively you could look at the code that manages the image corner
handles, that's probably a good reference for moving multiple things at
once!
…On Tue, Mar 12, 2019, 12:09 AM Sasha Boginsky ***@***.***> wrote:
@jywarren <https://github.com/jywarren> I was thinking of translation as
a subset of transform and that is the part I am stuck on. I definitely
think they should be moved into a leaflet layer, I was using the div
initially to check it works in dev tools and then tried creating a new
layer. I couldn't get it working and I think that's just because I am still
learning how to use and extend Leaflet. I was trying to create a
DistortableImagesOverlay.js file that mimics DistorableImageOverlay.js
and then I realized that doesn't make sense because the div is not an
image. Where do you suggest I should start? Thank you!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ7Lx-iSHN9fXoZm3NDrvSXfnQB7dks5vVyhGgaJpZM4bR9iJ>
.
|
|
@jywarren Under the hood it is all just CSS transformations. Ok I looked at a lot of resources today but I was all over the place because I started second guessing the entire implementation a little. If you agree with this implementation I will continue with it and narrow down my search to your suggestions! This should be a new layer, as in extending L.Layer? |
|
I believe that's right. But really do look at the handles code, I think it
may be a good model for group dragging. Even if it's all css, we can
structure the passing of location directives into each group member in a
similar way.
…On Tue, Mar 12, 2019, 12:28 AM Sasha Boginsky ***@***.***> wrote:
@jywarren <https://github.com/jywarren> Under the hood it is all just CSS
transformations. Ok I looked at a lot of resources today but I was all over
the place because I started second guessing the entire implementation a
little. If you agree with this implementation I will continue with it and
narrow down my search to your suggestions! This should be a new layer, as
in extending L.Layer?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ6gzBjx_sCqlq9nTixYtxYG9M1fSks5vVyzmgaJpZM4bR9iJ>
.
|
|
@jywarren got it thank you! |
|
I wonder if it's at all useful to think of making a Draggable object using the Leaflet API, to contain and update the whole collection? https://leafletjs.com/reference-1.4.0.html#draggable In any case, I am starting to think we want to make a new class, like Oh! Could we use FeatureGroup? https://leafletjs.com/reference-1.4.0.html#featuregroup |
|
@jywarren Yeah I will consider all of those. I have been reading through the documentation trying to figure out what is most appropriate. For Feature Group I was considering that one but I made a feature group to see how it works and it doesn't seem to create a new layer to group them all into or anything. It just allows you to set things like styling properties on one element that will be applied to all of the elements in the group. |
|
Awesome!!!
…On Fri, Mar 15, 2019, 12:14 AM Sasha Boginsky ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/select.html
<#158 (comment)>
:
> + objC.changes1 = obj.initVal.y - map.latLngToLayerPoint(img.getCorners()[0]).y;
+
+ objC.changes2 = obj.initVal1.x - map.latLngToLayerPoint(img.getCorners()[1]).x;
+ objC.changes3 = obj.initVal1.y - map.latLngToLayerPoint(img.getCorners()[1]).y;
+
+ objC.changes4 = obj.initVal2.x - map.latLngToLayerPoint(img.getCorners()[2]).x;
+ objC.changes5 = obj.initVal2.y - map.latLngToLayerPoint(img.getCorners()[2]).y;
+
+ objC.changes6 = obj.initVal3.x - map.latLngToLayerPoint(img.getCorners()[3]).x;
+ objC.changes7 = obj.initVal3.y - map.latLngToLayerPoint(img.getCorners()[3]).y;
+
+ window.objC = objC;
+ console.log('Changes: ', objC);
+
+
+ objD.newVal = L.point([obj2.initVal.x - objC.changes, obj2.initVal.y - objC.changes1]);
Ok officially have the two images moving together 👍👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ89-F5F3fBHBlcsuwGDHiq6z8suKks5vWx4agaJpZM4bR9iJ>
.
|
|
This is awesome - so what's the current mechanism to select, exactly? How you go into "selection mode" so to speak? Thank you! |
|
Looking to the next step, can we add to the README explaining some of this briefly, (esp the architecture a bit and how to initiate selection) and also, a small thing but can we make the selection color yellow and 0.5 opacity? This is SUPER exciting!!!! Thanks @sashadev-sky !!! |
|
@jywarren The images currently have a reference to For selection -- yes will change to yellow. Those lines are not Polylines yet, but I think that should go in a separate PR. For README I will push these changes in this PR once everything is done |
sashadev-sky
force-pushed
the
multiple_image_select
branch
from
d9c8e2d to
44d7ed1
Compare
|
@jywarren Ok let me know what you think now! |
|
It works now with the corner handles appearing in select.html! However, i don't see them working on |
|
oh lol i was just testing it 😄 It's incredible Sasha!!! |
There was a problem hiding this comment.
This will be so great. One thing we'll be able to do is that DistortableCollection can can have the correct-formatted json file generated from it, as required by our exporter services:
publiclab/mapknitter#326 (comment)
And broken down: publiclab/mapknitter-exporter-sinatra#1 (comment)
It should be able to produce a JSON file to POST up to the exporter, like the one we reference here:
|
@jywarren Oh it is because I forgot to add it to the featureGroup. Is it okay if the featureGroup is required for the functionality of the plugin, or did you want to keep it optional for people with only one image? I can either add the image to the group or change a few things around to make |
|
Hmm, I suppose we better make it optional, or we introduce a breaking change -- isn't that right? Sorry Sasha, i know you must be eager to get this merged! Thank you! |
|
@jywarren True! Ok no problem! |
|
@jywarren Ok -- by that I just mean they are available without the featureGroup too |
|
Looking really good. I found a few small issues but they aren't necessarily tied to your code, and can be fixed in follow-up:
So, I think this can be merged and we can address these (and any others we find) in follow-up, what do you think? |
| @@ -20,6 +20,10 @@ describe("L.RotateAndScaleHandle", function() { | |||
| }); | |||
| }); | |||
|
|
|||
| it.skip("Should not distort the image during scaling", function () { | |||
There was a problem hiding this comment.
We can add a test for this, I guess in a seperate issue? This is some amazing work @sashadev-sky! 🙌
There was a problem hiding this comment.
Thank you that's so nice!! yes but I'm not sure it should fully pass yet. because the image size passed in for edgeMinWidth is hardcoded, and the person can make edits to the image and change the shape and then scale. It would be a whole separate issue to work on for sure
|
🎉🎉🎉🙌🏽 |
|
@jywarren Should I save a draft for a new release for this version bump? Are there any specific guidelines I should follow to do that for this repo? |
|
Sure you can! Yeah, sorry I forgot Bower runs on releases. Once we switch
to Yarn we can just publish on npm.
I'd like to try to get the submenus stuff working before we publish to
mapknitter, since there are now so many new features. What do you think?
…On Sat, Apr 6, 2019, 7:29 AM Sasha Boginsky ***@***.***> wrote:
@jywarren <https://github.com/jywarren> Should I save a draft
<https://github.com/publiclab/Leaflet.DistortableImage/releases/new> for
a new release for this version bump? Are there any specific guidelines I
should follow to do that for this repo?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ4Ph4n4Lm19jJ7xH_v0KByjWCtIhks5veIUMgaJpZM4bR9iJ>
.
|
|
@jywarren Oh so repositories that use Bower typically post updated releases on Github and bower pulls in the latest release? Vs with Yarn we would stop publishing releases here and publish on NPM instead? Just wondering as I am new to this and want to learn! |
|
Yes, that's right. Bower is basically shut down now, one reason we are
shifting everything to Yarn! It was less centrally coordinated, and more
based on GitHub releases, but it influenced later systems like NPM and Yarn!
…On Sat, Apr 6, 2019 at 9:10 PM Sasha Boginsky ***@***.***> wrote:
@jywarren <https://github.com/jywarren> Oh so repositories that use Bower
typically post updated releases on Github and bower pulls in the latest
release? Vs with Yarn we would stop publishing releases here and publish on
NPM instead?
Just wondering as I am new to this and want to learn!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#158 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJzU2H9pDc8LeBqGTxPmTjYqbuKfqks5veUVggaJpZM4bR9iJ>
.
|
references #29, #30, #172, #85, #168, #166, #126, publiclab/mapknitter#60
In this PR:
extended
L.FeatureGroupto create a new classL.DistortableCollectionthat holds logic and state for a collection of selected images.created
L.Map.BoxSelectHandleto override and extend the defaultL.Map.BoxZoomsetting to zoom into the page by holding down shift and dragging. Instead theL.DistortableCollectionclass now listens for zoomend event and selects any image with any corner inside the zoombox bounds, passed fromL.Map.BoxSelectHandle. Has issues with event propagation, commented out event listener for nowdeselction and selection
- shift + drag selects all images in the bounding box (most accurate if drag from lower right to upper left and must drag over image corners) (part of commented out event)
test/src/DistortableCollectionSpec.jsfor selection / deselection of locked imagesimage manipulation functionalities:
misc. bugs
edgeMinWidthandedgeMinHeighton the image. This ensures that scaling doesn't scale to the point where image proportions are altered or hit 0 and throw an error (not complete implementation see pending)updates to README
pending: -- these will probably be new issues
edgeMinWidthandedgeMinHeightis currently hardcoded - also create doc on potential effects of scaling diff shapesUI for handling multiple imagesabstract tools UI from tools functions to allow for alternative UIs #140