Normalize keybindings

[sashadev-sky]

Fixes #226, #208, #219, #174, #211 (<=== 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
• code is in uniquely-named feature branch and has no merge conflicts 📁
• screenshots/GIFs are attached 📎 in case of UI updation
• @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!

===================================

1. RotateAndScaleHandles -> RotateScaleHandles

2. Rotate -> RotateScale

3. RotateStandalone -> Rotate

4. ToggleRotateDistort -> ToggleRotateScale (still a little confusing but at least now consistent)

5. _toggleIsolate keybinding temporarily (or maybe permanently) deleted

6. _toggleScale and _toggleRotate temporarily uncommented until they have their own toolbar keys. (see pending below)

7. hotkeys in multiple image select only toggle the state for one currently selected image (besides deselect all via escape / map click). This works through a _selected property that is assigned and managed in L.DistortableImage.Edit.js

8. Ensures that hotkeys don't override "locked" mode unless you explicitly toggleLock.

9. Fixes bug where the _removeOverlay hotkey triggers a confirm "you want to delete" multiple times (by the number of images you have) and then just deletes all of them at once.

10. Allow images to be initialized as "selected" #174: show toolbar and markers initially on one of the images - there is now an optional selected: true option

11. Incorporate newer key API method in place of the deprecated which method #211 Incorporate newer key API method in place of the deprecated which method

12. adds testing for both single and multiple image selection / deselection (including testing for images that are locked)

13. Added documentation in README for new selected option, plus improved documentation of all available options
    ===================================

PENDING:

• Confirm with @rexagod about reverting his polyfill for now - pending confirmation

• open a new PR to fix a deletion bug pointed out by @rexagod in Bug: Multiple image select with boxZoom #186. Point 9 here solved this bug for deletion using its keybinding but not for deletion using the toolbar action.

• update keymapper to reflect updates made here

• allow for rotateScale mode once @rexagod PR add default selector Add default selector #206 is merged

• open new PR for @rexagod to add scale, rotate, toolbar icons then uncomment the keybindings for those? For now, the icon that is mean for rotating is RotateScale and therefore that keybinding is not uncommented. But it will be up to you which keybindings pair with which icons as long as they all have one.

  • In original issue @jywarren recommends: -

  Let's just say as a rule, we don't expose or make available the separated Rotate and Scale tools at all, as they're only for (edited) Publish changes compatible with downstream React wrapper #105 usage, and we always assume the combined RotateAndScale tool.

  • But after further discussion decided to keep these tools, so the next step is just to add toolbar keys for them. For now I have uncommented their keybindings

[sashadev-sky]

@jywarren please review! I think this would be easier if you just pulled it and looked at it rather than uploading GIFs?

Before wrote:
I turned off all of the hotkeys for multiple image select except for deselect all on escape / map click
......

taking this back now:
instead hotkeys in multiple image select only toggle the state for one currently selected image now (besides deselect all via escape / map click). This works through a _selected property that is assigned and managed in L.DistortableImage.Edit.js

I think that if we want to introduce multiple image actions, we need to introduce breaking changes of requiring a feature group, and letting it manage all the toggle actions. Otherwise it gets more and more complicated to deal with event propagation and UI updates. This will reduce a lot of bugs we have in our code too, and we can stop developing for two completely separate cases (index.html vs select.html)

@rexagod what are your thoughts

[sashadev-sky]

Also @jywarren what did you mean by issue #174? Just that the markers and toolbar will be on for one image initially? This was a really quick fix here, I just commented out the code that hides the toolbar / markers initially. Please let me know if this is what you were looking for! (Once again easier to test for yourself at this point I think)

[sashadev-sky]

@jywarren - regarding #174 - is this what you would like: in single image and multiple image interface one image will always be displayed as selected right when the page loads (that is, handlers and toolbar). This is not counting locked images, whose handles are always immediately present

This will just be the default way it works. I think this is fine, rather than passing in option to make it do this. But really up to you

[jywarren]

I think not everyone will want this to be the case, so I had hoped for a constructor parameter like `isSelected` to set to true, but defaulting false. Could we do this? Thank you!

…

On Tue, Apr 30, 2019 at 6:51 PM Sasha Boginsky ***@***.***> wrote: @jywarren <https://github.com/jywarren> - regarding #174 <#174> - is this what you would like: in single image and multiple image interface one image will always be displayed as selected right when the page loads (that is, handlers and toolbar). This is not counting locked images, whose handles are always immediately present [image: Screen Shot 2019-04-30 at 6 47 33 PM] <https://user-images.githubusercontent.com/41092741/56998221-8994f900-6b78-11e9-8a9c-8fab13563cd6.png> [image: Screen Shot 2019-04-30 at 6 47 17 PM] <https://user-images.githubusercontent.com/41092741/56998268-b77a3d80-6b78-11e9-9282-5ee53e6ea2e0.png> This will just be the default way it works. I think this is fine, rather than passing in option to make it do this. But really up to you — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J23BODAUGGZ722WORLPTDEM7ANCNFSM4HHKYREA> .

[sashadev-sky]

I think not everyone will want this to be the case, so I had hoped for a constructor parameter like isSelected to set to true, but defaulting false. Could we do this? Thank you!

No problem! Lock will stay as is though. Is that fine? I think locked images should show they are locked at all times. I will update new UI once changes are made @jywarren

[jywarren]

yep, although I believe there are circumstances where locked images need to not have corners marked -- very few, but some -- for example, when people display maps in a totally non-editable context. Example could be: when embedding your map on a webpage. See "embed code" here: https://mapknitter.org/maps/cranston-test and the URL it's tied to, here: http://mapknitter.org/embed/cranston-test -- it's a read-only view, does that make sense? Thank you!!!

…

On Tue, Apr 30, 2019 at 7:04 PM Sasha Boginsky ***@***.***> wrote: I think not everyone will want this to be the case, so I had hoped for a constructor parameter like isSelected to set to true, but defaulting false. Could we do this? Thank you! No problem! Lock will stay as is though. Is that fine? I think locked images should show they are locked at all times. I will update new UI once changes are made — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JZFL7VH634KZGSNXWTPTDF4HANCNFSM4HHKYREA> .

[sashadev-sky]

@jywarren not sure I am really understanding, but if the image is locked and you click 'embed code', it just unlocks it. So is it fine to continue with the same logic I have or is there something else I should change.

[jywarren]

Sorry, i should've tried explaining more. Basically, when people make maps, sometimes they want to share them on a blog or on PL.org, not in an editable state, but just distorted and not interactive at all (besides being able to pan the overall map). So we have this "embed code" field here:

And that shows an iframe with this view, which as you can see is totally not editable: https://mapknitter.org/embed/cranston-test

For that use case, the images are "locked" essentially, but we don't show any UI on top of them at all, because they are essentially "part of the map". Make sense? Thanks!

[sashadev-sky]

@jywarren Ok four scenarios for you to see here to decide if this is exactly what you are looking for, and then I will add doc. / testing:

index.html (single image interface)

• scenario 1 image not initialized with selected: true passed

• scenario 2 image initialized with selected: true passed

select.html (multiple image interface)

• scenario 3 no images initialized with selected: true passed (note behavior of lock)

• scenario 4 one image initialized with selected: true passed

• scenario 5 two images initialized with selected: true passed - the same image as before and another one. Collection group ensures that even if you pass selected: true to multiple images, only the last one added to the group will actually be selected

• scenario 6 an image with mode: lock is passed selected: true:

[sashadev-sky]

Sorry, i should've tried explaining more. Basically, when people make maps, sometimes they want to share them on a blog or on PL.org, not in an editable state, but just distorted and not interactive at all (besides being able to pan the overall map). So we have this "embed code" field here:

And that shows an iframe with this view, which as you can see is totally not editable: https://mapknitter.org/embed/cranston-test

For that use case, the images are "locked" essentially, but we don't show any UI on top of them at all, because they are essentially "part of the map". Make sense? Thanks!

This has nothing to do with the initialization code here though, right? That logic is placed further down the line in mapknitter?

[jywarren]

That's right, just responding to your comment that lock handles would always be shown. And your scenarios look perfect!!!! Awesome.

…

On Tue, Apr 30, 2019, 10:57 PM Sasha Boginsky ***@***.***> wrote: Sorry, i should've tried explaining more. Basically, when people make maps, sometimes they want to share them on a blog or on PL.org, not in an editable state, but just distorted and not interactive at all (besides being able to pan the overall map). So we have this "embed code" field here: [image: image] <https://user-images.githubusercontent.com/24359/57000550-43449780-6b82-11e9-9672-4a6cae30c2b6.png> And that shows an iframe with this view, which as you can see is totally not editable: https://mapknitter.org/embed/cranston-test For that use case, the images are "locked" essentially, but we don't show any UI on top of them at all, because they are essentially "part of the map". Make sense? Thanks! This has nothing to do with the initialization code here though, right? That logic is placed further down the line in mapknitter? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JYOHPYYZNSOWNEICJ3PTEBFTANCNFSM4HHKYREA> .

[jywarren]

This is great. Thanks for the detailed summary, and go ahead and merge!

Let's release a new version, and ensure it gets pulled into MapKnitter and appears on http://mapknitter-unstable.laboratoriopublico.org (from stable branch there!)

[jywarren]

er, unstable branch!

[rexagod]

@sashadev-sky, I'm okay with the polyfill removed as long as all the tests pass, also I'm looking to implement async functions in the future if any such issue arises. So yeah, this looks good to merge!

[sashadev-sky]

@jywarren sounds good I just made a PR to bump the version I will tag you there for review then draft new release

[sashadev-sky]

@jywarren Ok I drafted a release let me know your thoughts on going forward with mentions in the releases! You can remove that part if you prefer not to start doing that.

Also, what is the easiest way to create a tarball?

[jywarren]

Hmm, I think it automatically creates a tgz file, why do you ask?

…

On Tue, May 7, 2019, 9:32 PM Sasha Boginsky ***@***.***> wrote: @jywarren <https://github.com/jywarren> Ok I drafted a release let me know your thoughts on going forward with mentions in the releases! You can remove that part if you prefer not to start doing that. Also, what is the easiest way to create a tarball? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J2BY4RUC5IQW7WE6Z3PUIURVANCNFSM4HHKYREA> .

[sashadev-sky]

@jywarren Oh I see the assets are there now! I didn't realize they were autogenerated, I refreshed the page after I posted the draft but I'm realizing now that they are probably then generated automatically when you hit publish