Control toolbar, keymap guide, and util fn to create custom toolbars #147
Conversation
There was a problem hiding this comment.
OK i see how this is different from #155 now; this allows for new instances of L.control to be added to the map. However, we should see if that's what #140 needs, or if we need something even broader. @sashadev-sky - in the ideas around an Image Manager UI that is potentially built in React, would that be able to fit within the idea of a Leaflet Control?
https://leafletjs.com/reference-1.4.0.html#control
And if so, could it be passed into the lib using addToolbar here?
Or perhaps we don't need to sweat this because the Image Manager would be a UI to manipulate a collection of images, like #158 #29 describe, and it would just get it's own toolbar UI that wouldn't live inside the individual DistortableImage instances?
Thanks, just making sure I understand the changes in this PR and how they relate to other changes. I appreciate everyone's work here! 👍 👍 👍
|
Hi @rexagod can you reply above and rebase this so we can take a fresh clean look at the changes for just this PR? Thank you!!! |
|
@jywarren I think we have been diverging into two separate options: individual image toolbars and an image collection manager toolbar (that would still be able to be used with only one image). I was imagining the image collection manager to not rely on individual instances because, for example, if editing multiple images so far my plan has been to move them inside of a container element or layer that the edits will be applied to. It is also possible to create an implementation where the toolbar knows which images are selected and applies the transformations on all of them at once, which would make it reliant on them. So this is dependent on how multiple image select is implemented |
|
I agree - we want to preserve the individual toolbars. But the "group of images toolbar" is distinct. Thank you! |
|
Hi @sashadev-sky - unfortunately this is now out of sync again... @rexagod is in exams, do you think you could help try to get this rebased and ready to merge? I'm sorry, we've been pushing ahead so hard on multiple image selection that I didn't merge this when we had an opportunity. Thanks all!!! |
|
@jywarren Yes will do! Is it urgent? Is Friday when you usually publish changes in all your repos? |
|
Not urgent - i just feel bad that we haven't gotten this one in, and don't
want to bypass it again! Thank you so much. And, we haven't published
MapKnitter for a little while as we're getting the containerization all set
up. So no rush with regard to publishing to production, thank you!
…On Fri, Apr 5, 2019 at 2:45 PM Sasha Boginsky ***@***.***> wrote:
Yes will do! Is it urgent? Is Friday when you usually publish changes in
all your repos?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#147 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ8qVrkVOv6QapdQjRl939q9FU4zsks5vd5mxgaJpZM4bIq-y>
.
|
|
No problem I am really excited to pull this and see how it works! |
|
@jywarren @rexagod ok here is what I think should be done: Open a PR just for adding a keymapper guide. Don't mention the custom toolbar at all yet. Then we can ensure the keymapper is clear and makes sense and documented well in the README and then move on from there. @rexagod can you open a keymapper PR? |
Co-Authored-By: rexagod <rexagod@gmail.com>
Co-Authored-By: rexagod <rexagod@gmail.com>
Co-Authored-By: rexagod <rexagod@gmail.com>
Co-Authored-By: rexagod <rexagod@gmail.com>
|
Submitting a keymapper docs PR ASAP! |
|
@sashadev-sky Done at #212! |
|
Can this be closed in favor of #212 or can we try to open a narrower PR with what's left here? Thank you! |
|
@jywarren I will review keymapper again tomorrow, we will know after that is merged exactly what needs to be done here I guess |
|
I agree with @jywarren on closing this. We can still extract any relevant code from this branch in a separate PR after that. |
Re: #140
@publiclab/reviewers @jywarren