Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hotkeys and major file restructuring #393

Merged
merged 54 commits into from
Sep 15, 2019
Merged

Hotkeys and major file restructuring #393

merged 54 commits into from
Sep 15, 2019

Conversation

sashadev-sky
Copy link
Member

@sashadev-sky sashadev-sky commented Aug 20, 2019 •
edited
Loading

Fixes #376 (<=== Add issue number here)
Fixes #360

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 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!

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

changes:

File Restructuring:

  • made a handles folder in edit folder, which contains EditHandle.js, LockHandle.js, etc.
  • made a toolbars folder to hold DistortableImage.PopupBar.js and DistortableImage.ControlBar.js
  • made an actions folder in edit folder, which follows a similar convention:
Orig. Name New Name File old key new key old method new method comments
L.EditAction EditAction.js
var ToggleTransparency L.OpacityAction OpacityAction.js t o _toggleTransparency _toggleOpacity Swapped icons so they represent the state currently in.
var ToggleOutline L.BorderAction BorderAction.js o b _toggleOutline _toggleBorder Swapped. Also, image outline refers to something else in Leaflet & most code convention - focusable ring around an el. border is clearer.
var Delete and var Deletes L.DeleteAction DeleteAction.js backspace backspace methods are the same just combined plural & singular version after standardizing the toolbar API w/ L.DistortableCollection.Edit now just uses conditionals to decide if call should be plural or singular
var ToggleRotate L.RotateAction RotateAction.js CAPS LOCK r because CAPS LOCK as a keybinding is a bug. _toggleRotate _rotateMode it doesn't make sense to toggle every mode with distort, or any single mode. Then icons would need to all toggle to same icon. TB wouldn't make sense. Also overcomplicates current / future mode API: ex. selecting diff default mode or just forbidding 'distort'.
var ToggleRotateScale L.FreeRotateAction FreeRotateAction.js r, d f _toggleRotateScale _freeRotateMode doesn't toggle anymore either
N/A L.DistortAction DistortAction.js N/A d N/A _distortMode
var ToggleScale L.ScaleAction ScaleAction.js s s _toggleScale _scaleMode
var ToggleLock and var Locks L.LockAction LockAction.js l l, u - l only locks, u only unlocks, button still toggles _toggleLock _togglelockMode
var Export and var Exports L.ExportAction ExportAction.js N/A e
var EnableEXIF L.GeolocateAction GeolocateAction.js
var Revert L.RevertAction RevertAction.js
var ToggleOrder L.StackAction StackAction.js j, k q, a now q will only stack up / a will only stack down, not random

  • updated Keymapper accordingly

  • In a past PR, when I added an L.DistortableCollection.Edit DistortableCollection.Edit #335, this allowed me to align the Tool / Action APIs in L.DistortableCollection and L.DistortableImageOverlay..

    • L.DistortableCollection now shares its tools with L.DistortableImageOverlay. L.ExportAction, L.DeleteAction, and L.LockAction are all written to work for single and multi interface to avoid having to write duplicate tools.
    • was able to get rid of var Exports, var Deletes, and var Locks.
    • the only one L.DistortableCollection still doesn't share is var Unlocks, which is now L.UnlocksActions (kept it in ControlBar file instead of giving it its own)

  • bump version to 0.8.0

  • distort is finally its own toolbar action, so and most icons aren't toggle actions.

UI Improvements:

  • mode tools are highlighted according to their corresponding color when active

ui 2

Bug Fixes:

  • Keybindings are only present on individual images that have the corresponding toolbar action. Same with collection group.
  • Rotate keybinding changed from CAPS LOCK to r
  • you can switch out of lock mode via keybindings for a mode or clicking on another mode
  • on image doubleclick, it will iterate through all available modes in order.
  • after exiting out of lock mode, you will be on the mode that you originally set as your default (or 'distort' - our default)
  • README updates

Pending:

@sashadev-sky
Copy link
Member Author

@rexagod i'm gonna finish off the 2nd half for #376!

@sashadev-sky
Copy link
Member Author

@jywarren @rexagod this is ready for review! This fixes the keybindings issue where they don't match with the actual tools in the images individual / collective toolbars, and addresses #161 in an organized way!

@sashadev-sky
Copy link
Member Author

ill update docs if you guys like this

@sashadev-sky
Copy link
Member Author

@jywarren note that I went about this a little differently than in #317. Primarily because the actions work for single and multiple instances of images with conditionals

@sashadev-sky sashadev-sky mentioned this pull request Aug 21, 2019
Closed
rexagod
rexagod approved these changes Aug 21, 2019
View reviewed changes
Copy link
Member

@rexagod rexagod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the approach here! LGTM! 🎉

@sashadev-sky
Copy link
Member Author

@jywarren this is ready for merge (just needs rebase) and approved by rexagod! But I will hold on that because it is a big change - I made a markdown table in the PR description so you can easily follow the changes and new structure. Let me know your thoughts! For now going to revert my focus on getting MK to work

@sashadev-sky
Copy link
Member Author

@jywarren just wanted to follow up here because this PR makes some big changes!

@jywarren
Copy link
Member

jywarren commented Aug 30, 2019 via email

@sashadev-sky
Copy link
Member Author

sashadev-sky commented Aug 30, 2019 via email

@jywarren
Copy link
Member

jywarren commented Aug 30, 2019 via email

@sashadev-sky
Copy link
Member Author

@jywarren ok got it!

@themacboy
Copy link
Contributor

themacboy commented Sep 5, 2019 •
edited
Loading

Hello @sashadev-sky, I tryed to figure how to add my Drag only mode in this PR. I set a few changes, but im not sure if Im doing all correctly.

For example, i don't understand how to add my 4 dot start icon in the L.ToolbarIconSet. (sorry not sure how this draw code works).

I hope I helped and not make your work more difficult.

Btw, Im not sure if I must code in your branch the handles changes I have made in my PR, or if you can reqauest it automatically from github (still trying to learn how GIT works).

@sashadev-sky
Copy link
Member Author

sashadev-sky commented Sep 7, 2019 •
edited
Loading

@themacboy sorry I will checkout your work soon :) working on a critical problem with setup downstream right now.

Have you taken a look at this wiki?
https://github.com/publiclab/Leaflet.DistortableImage/wiki/SVG-Icon-System

don't worry about my branch. If yours is ready to merge before this is reviewed I will just rebase and reformat it here to match.

@sashadev-sky sashadev-sky merged commit 85520ed into publiclab:main Sep 15, 2019
This was referenced Sep 15, 2019
Closed
Merged
@sashadev-sky
Copy link
Member Author

@jywarren really proud of this refactor looks so much more aesthetic

Leaflet.DistortableImage/src/edit/DistortableImage.Edit.js

Lines 289 to 313 in d1cd4ff

_scaleMode: function() {
if (!this.hasTool(L.ScaleAction)) { return; }
this._setMode('scale');
},
_distortMode: function() {
if (!this.hasTool(L.DistortAction)) { return; }
this._setMode('distort');
},
_rotateMode: function() {
if (!this.hasTool(L.RotateAction)) { return; }
this._setMode('rotate');
},
_freeRotateMode: function() {
if (!this.hasTool(L.FreeRotateAction)) { return; }
this._setMode('freeRotate');
},
_toggleLockMode: function() {
if (!this.hasTool(L.LockAction)) { return; }
if (this._mode === 'lock') { this._unlock(); }
else { this._lock(); }
},

@sashadev-sky sashadev-sky mentioned this pull request Sep 15, 2019
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jywarren jywarren jywarren approved these changes

@rexagod rexagod rexagod approved these changes

Assignees
No one assigned
Labels
API bug needs-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hotkeys should only be available for present toolbar actions Aftert unlock set mode is not acurate ...
4 participants
@sashadev-sky @jywarren @themacboy @rexagod