Keymapper phase 2 enhancements #243

rexagod · 2019-05-03T15:16:34Z

Fixes #239

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
code is in uniquely-named feature branch and has no merge conflicts 📁

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!

rexagod · 2019-05-03T15:19:13Z

Next up

"along the way lets figure out how to make keymapper_options and keymapper more convenient, perhaps merge them into one option."
"Update docs"

/cc @jywarren @sashadev-sky

rexagod · 2019-05-04T10:00:59Z

Okay so how I've merged keymapper_position and keymapper is in a position param that will render the keymapper only when there's a non-null value passed to it.

 keymapper = new L.DistortableImage.Keymapper(map,img., {
        position: '' //keymapper not rendered
      });

rexagod · 2019-05-04T10:06:27Z

Open to other ideas regarding merging both these params, else I guess maybe we can proceed with this?

sashadev-sky

@rexagod
Ok so looking over this I am wondering about a few things. We might want to set up a time to Zoom to go over it if you'd like.

My main question is why make the keymapper extend the handler class? I see that it gives us the ability to choose whether the keymapper is placed on the map or not depending on whether we leave the line keymapper.enable() uncommented. But I don't see how this is different from having the keymapper extend L.control, and in the index.html file initializing it just like this:

new L.DistortableImage.Keymapper({position: 'bottomleft'}).addTo(map);

and commenting that line out if you don't want to have the keymapper on the page. This takes care of the position problem if we just add an L.setOptions(this, opts) to its initialize function.

We would have to get rid of the keymapper: false option for now but I think thats fine. If we really wanted to we could move this option to work with the keymapper class.

I was also unsure about its functionality because I found that after the keymapper is initially enabled, running keymapper.disable() does not remove it from the map, as img.editing.disable() removes the markers and toolbars from an image.

Would love some insight here because I could be misunderstanding the point! Also I think @jywarren initially suggested used L.handler. Was there a specific reason for that?

rexagod · 2019-05-25T19:40:12Z

In response to #258

cc @jywarren

rexagod · 2019-05-25T19:41:29Z

@sashadev-sky I've noted your suggestions and will chime in a bit after I wrap up the CLI tool if it's not a problem? Thanks!

jywarren · 2019-05-25T19:43:47Z

Oh this looks good! Thanks! If this is getting ready for a merge and release, let me know but if it'll be another little while, we could consider just using an "x" button, what do you think?

…

On Sat, May 25, 2019, 3:42 PM Pranshu Srivastava ***@***.***> wrote: @sashadev-sky <https://github.com/sashadev-sky> I've noted your suggestion and will chime in a bit after I wrap up the CLI tool if it's not a problem? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243?email_source=notifications&email_token=AAAF6JYRME2WWWYBOJQSE4TPXGI6TA5CNFSM4HKUZDD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWHX7CI#issuecomment-495943561>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J6FM2KUXYM6HT4D65TPXGI6TANCNFSM4HKUZDDQ> .

rexagod · 2019-05-25T19:54:55Z

Okay, no problem, and just to be clear, where should I place this 'x' button? Do you think that maybe we should use a fontawesome icon inside the button above (replace 'disable keymapper' with 'x' icon) or is there any particular position that would make more sense?

jywarren · 2019-05-25T19:56:58Z

Yeah, a fontawesome icon sounds... awesome! And yeah, I guess upper corner would be perfect. Thanks!

…

On Sat, May 25, 2019, 3:55 PM Pranshu Srivastava ***@***.***> wrote: Okay, no problem, and just to be clear, where should I place this 'x' button? Do you think that maybe we should use a fontawesome icon inside the button above (replace 'disable keymapper' with 'x' icon) or is there any particular position that would make more sense? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243?email_source=notifications&email_token=AAAF6J46DKWMBF24OQNCSXDPXGKQ7A5CNFSM4HKUZDD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWHYFPI#issuecomment-495944381>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J4NWCTIBZTMXXI3NZTPXGKQ7ANCNFSM4HKUZDDQ> .

rexagod · 2019-05-26T04:55:27Z

@jywarren How does this look?

sashadev-sky · 2019-05-26T22:59:38Z

@jywarren @rexagod do you think ability to hide keymapper should be addressed in a separate PR? And we can use a toggled icon for minimizing and expanding it? Because deleting is a permanent action and that might confuse people

also still need to review the implementation for this with @rexagod!

rexagod · 2019-05-29T21:26:32Z

Would love some insight here because I could be misunderstanding the point! Also I think @jywarren initially suggested used L.handler. Was there a specific reason for that?

Hmm. I guess his response here should definitely pave the way for this PR. Pinging @jywarren.

I was also unsure about its functionality because I found that after the keymapper is initially enabled, running keymapper.disable() does not remove it from the map, as img.editing.disable() removes the markers and toolbars from an image.

@sashadev-sky disable() seems to work, or am I running a different use case scenario?

jywarren · 2019-05-29T22:20:43Z

I'm fine with splitting things out, but had asked @rexagod to prioritize the "close" button because it's causing some difficulties in live MapKnitter.org. Thank you! As to handler... gosh, i feel like it was a million years ago. I honestly don't remember, but if you can pull the link to the comment i can try to catch up on it?

…

On Wed, May 29, 2019 at 5:27 PM Pranshu Srivastava ***@***.***> wrote: Would love some insight here because I could be misunderstanding the point! Also I think @jywarren initially suggested used L.handler. Was there a specific reason for that? Hmm. I guess his response here should definitely pave the way for this PR. Pinging @jywarren <https://github.com/jywarren>. I was also unsure about its functionality because I found that after the keymapper is initially enabled, running keymapper.disable() does not remove it from the map, as img.editing.disable() removes the markers and toolbars from an image. @sashadev-sky <https://github.com/sashadev-sky> disable() seems to work, or am I running a different use case scenario? [image: test] <https://user-images.githubusercontent.com/33557095/58592544-52545d80-8286-11e9-9fd5-614884abe05f.gif> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243?email_source=notifications&email_token=AAAF6J3BO5UE77LVWDPFQYDPX3YIRA5CNFSM4HKUZDD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWQWA5A#issuecomment-497115252>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J4IOLFZKDHDJT3BYNTPX3YIRANCNFSM4HKUZDDQ> .

sashadev-sky · 2019-05-30T00:55:15Z

@jywarren #212 (comment) I think it might have been on accident you suggested but I could be wrong!

@rexagod hmm I think what I was referring to was that if you try to use disable() later on, like in the console perhaps, it doesn't disable the keymapper like it would with the handles.

If there isn't an identifiable benefit to making it in a handler class that then call the control class to create it, should we just refactor to make it control class like in my suggestion above?

publiclab/Leaflet.DistortableImage#243

jywarren · 2019-05-30T18:55:45Z

I just turned off the keymapper in MapKnitter briefly until we get the "many keymappers appearing" and "ability to close keymapper" resolved: #243

Thanks!

I also thought we ought to make it not /ever/ open a 2nd keymapper, does that make sense?

Awesome!

jywarren · 2019-05-30T18:56:02Z

That should relieve pressure on this PR so we can take a bit more time to get it right. Thanks, all!

jywarren · 2019-05-30T18:58:12Z

jywarren · 2019-05-30T19:01:08Z

Also I think @jywarren initially suggested used L.handler. Was there a specific reason for that?

Aha! So, in #212 (comment), I meant that we could give it its own namespace, /like/ the L.Handler example, but didn't mean that it should actually be named Handler. So, L.Keymapper maybe, or if it's very LDI specific, maybe L.DistortableImageKeymapper -- or even L.DistortableImage.Keymapper... although that then isn't as modular again. Make sense?

jywarren · 2019-05-30T19:02:34Z

dist/leaflet.distortableimage.js

-L.DistortableImage.Keymapper = L.Control.extend({
-    initialize: function(options) {
-        L.Control.prototype.initialize.call(this, options);
+L.DistortableImage.Keymapper = L.Handler.extend({


So here, yeah - sorry, we don't need this to extend Handler - that was just a fragment of the code i was quoting. I mainly meant just putting this all in L.DistortableImage.Keymapper - which looks great!

jywarren · 2019-05-30T19:03:13Z

Sorry, clarifying further re: Handler, i put an inline comment at #243 (comment)

publiclab/Leaflet.DistortableImage#243

rexagod · 2019-06-01T21:14:09Z

And we can use a toggled icon for minimizing and expanding it? Because deleting is a permanent action and that might confuse people

I think what I was referring to was that if you try to use disable() later on, like in the console perhaps, it doesn't disable the keymapper like it would with the handles.

I also thought we ought to make it not /ever/ open a 2nd keymapper, does that make sense? -- Sasha review: confirming this is fixed!

@jywarren @sashadev-sky Okay so I've added the aforementioned changes to this, while holding on to the Handler -> Control one, for the following reasons.

No Hooks.
No enable/enabled methods.
Leaflet's doc on L.Control: "L.Control is a base class for implementing map controls. Handles positioning. All other controls extend from this class." Now in case of keymapper, I really don't think we need to "control" the map in any way (like the zoom buttons at the topleft), just the keymapper itself. As far as the positioning is concerned, we can already do that by passing the position parameter at instantiation.

Therefore, I didn't see any benefit from the refactoring, so I thought of discussing this out first. But I can be wrong, and thus ask your views on the same.

Thanks!!

rexagod · 2019-06-01T22:00:51Z

Also, I'm currently using Handler wrapper class just for the above mentioned functionality it provides, while the actual keymapper is implemented in Control class itself (refer CSS class below).

var keymapper = L.control({position: this._position});
        keymapper.onAdd = function() {
            var el_wrapper = L.DomUtil.create("div", "l-container");
            el_wrapper.setAttribute('id', 'keymapper-wrapper');
            el_wrapper.innerHTML = "<table><tbody>" +
            "<tr><td><center><span id='keymapper-heading'>Keymappings</span></center></td></tr>" +
            "<tr><td id='keymapper-hr'><hr></td></tr>" +
            "<tr><td><kbd>j</kbd>, <kbd>k</kbd>: <span>Send up / down (stacking order)</span></td></tr>" +
            "<tr><td><kbd>l</kbd>: <span>Lock</span></td></tr>" +
            "<tr><td><kbd>o</kbd>: <span>Outline</span></td></tr>" +
            "<tr><td><kbd>s</kbd>: <span>Scale</span></td></tr>" +
            "<tr><td><kbd>t</kbd>: <span>Transparency</span></td></tr>" +
            "<tr><td><kbd>d</kbd> , <kbd>r</kbd>: <span>RotateScale</span> </td></tr>" +
            "<tr><td><kbd>esc</kbd>: <span>Deselect All</span></td></tr>" +
            "<tr><td><kbd>delete</kbd> , <kbd>backspace</kbd>: <span>Delete</span></td></tr>" +
            "<tr><td><kbd>caps</kbd>: <span>Rotate</span></td></tr>" +
            "<tr><td><center><button id='close-keymapper-button'>" + 
            "<kbd id='close-keymapper-kbd'>" + 
            "<i class='fa fa-times-circle' aria-hidden='true'></i>" + 
            "</kbd></button></center></td></tr>" +
            "</tbody></table>";
            return el_wrapper;
        };
        keymapper.addTo(this._map);

sashadev-sky · 2019-06-05T03:44:50Z

@rexagod I am going to pull this and rebase you because you've been out of the repo for a bit and then I will go over it!

rexagod

The new look is awesome, @sashadev-sky! Congrats on fixing the Travis build, too! It can sure be quite frustrating at times!

LGTM! 😃🙌

publiclab/Leaflet.DistortableImage#243

sashadev-sky added the needs-review label May 4, 2019

rexagod changed the base branch from main to gh-pages May 5, 2019 12:25

rexagod changed the base branch from gh-pages to main May 5, 2019 12:26

sashadev-sky reviewed May 17, 2019

View reviewed changes

jywarren added a commit to publiclab/mapknitter that referenced this pull request May 30, 2019

briefly turn off keymapper until bug is resolved

a7ecf32

publiclab/Leaflet.DistortableImage#243

jywarren mentioned this pull request May 30, 2019

briefly turn off keymapper until bug is resolved publiclab/mapknitter#651

Merged

jywarren reviewed May 30, 2019

View reviewed changes

jywarren added a commit to publiclab/mapknitter that referenced this pull request May 30, 2019

briefly turn off keymapper until bug is resolved (#651)

cc7e924

publiclab/Leaflet.DistortableImage#243

Control toolbar + Keymap guide + Util fn to make custom toolbars

7a6822c

sashadev-sky added 22 commits July 29, 2019 22:19

More refactoring

a6f30e4

changge from button to a

7c3b715

updates

ff6c939

clean up

9f83462

clean up

59b93d7

fix broken package-lock

027fd01

bump version

abb18e4

position working and ui updates

41df974

modal

53f5526

refactor

75384f0

get rid of arrow collapse svg

664390e

finish cleanup

d8d029d

return extra line to css file

a96fbb1

cleanup

13a4442

update README

585dbcf

syntax

9984530

try to resolve npm travis trouble

37f3eb3

try npm rebuild

f8497fa

try nuking cache

7bd470f

add node v 12 to .travis.yml

fac251a

reorder dependencies undo gruntfile change

2c17fc7

rebase again, readd a line deleted

a1b3002

sashadev-sky force-pushed the keymapper-phase-2 branch from 7e2dc43 to a1b3002 Compare July 30, 2019 02:27

sashadev-sky added 2 commits July 29, 2019 22:42

update README

00db91a

try updaing paths

8d92e0b

rexagod commented Jul 30, 2019

View reviewed changes

sashadev-sky merged commit 0e68573 into publiclab:main Jul 30, 2019

This was referenced Jul 30, 2019

Ability to hide keymapper? #258

Closed

Keymapper Pt. 3 - Dynamic #368

Open

chen-robert pushed a commit to chen-robert/mapknitter that referenced this pull request Dec 5, 2019

briefly turn off keymapper until bug is resolved (publiclab#651)

8e4c479

publiclab/Leaflet.DistortableImage#243

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keymapper phase 2 enhancements #243

Keymapper phase 2 enhancements #243

rexagod commented May 3, 2019

rexagod commented May 3, 2019

rexagod commented May 4, 2019

rexagod commented May 4, 2019

sashadev-sky left a comment •

edited

Loading

rexagod commented May 25, 2019

rexagod commented May 25, 2019 •

edited

Loading

jywarren commented May 25, 2019 via email

rexagod commented May 25, 2019

jywarren commented May 25, 2019 via email

rexagod commented May 26, 2019

sashadev-sky commented May 26, 2019

rexagod commented May 29, 2019

jywarren commented May 29, 2019 via email

sashadev-sky commented May 30, 2019

jywarren commented May 30, 2019

jywarren commented May 30, 2019

jywarren commented May 30, 2019

jywarren commented May 30, 2019

jywarren May 30, 2019

jywarren commented May 30, 2019

rexagod commented Jun 1, 2019 •

edited by sashadev-sky

Loading

rexagod commented Jun 1, 2019 •

edited

Loading

sashadev-sky commented Jun 5, 2019

rexagod left a comment

Keymapper phase 2 enhancements #243

Keymapper phase 2 enhancements #243

Conversation

rexagod commented May 3, 2019

rexagod commented May 3, 2019

Next up

rexagod commented May 4, 2019

rexagod commented May 4, 2019

sashadev-sky left a comment • edited Loading

Choose a reason for hiding this comment

rexagod commented May 25, 2019

rexagod commented May 25, 2019 • edited Loading

jywarren commented May 25, 2019 via email

rexagod commented May 25, 2019

jywarren commented May 25, 2019 via email

rexagod commented May 26, 2019

sashadev-sky commented May 26, 2019

rexagod commented May 29, 2019

jywarren commented May 29, 2019 via email

sashadev-sky commented May 30, 2019

jywarren commented May 30, 2019

jywarren commented May 30, 2019

jywarren commented May 30, 2019

jywarren commented May 30, 2019

jywarren May 30, 2019

Choose a reason for hiding this comment

jywarren commented May 30, 2019

rexagod commented Jun 1, 2019 • edited by sashadev-sky Loading

rexagod commented Jun 1, 2019 • edited Loading

sashadev-sky commented Jun 5, 2019

rexagod left a comment

Choose a reason for hiding this comment

sashadev-sky left a comment •

edited

Loading

rexagod commented May 25, 2019 •

edited

Loading

rexagod commented Jun 1, 2019 •

edited by sashadev-sky

Loading

rexagod commented Jun 1, 2019 •

edited

Loading