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.

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

Modes API (actual) #442

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Modes API (actual) #442

wants to merge 25 commits into from

Conversation

sashadev-sky
Copy link
Member

@sashadev-sky sashadev-sky commented Sep 23, 2019

Fixes #296 (<=== Add issue number here)
Fixes #400
Fixes #398

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:

  • Improved selection and collection UI
  • allows the user to select what modes are available on the image and updates documentation on doing so. Image can also have a blank mode.
  • also getModes, getMode, isMode, hasMode exposed to API
  • getCollected, lockGroup, unlockGroup
  • all possible handles are stored on class as static L.DistortableImage.Edit.HANDLES, but this._handles on an individual image will now only ever reflect the handles that the image should have based on which modes it has.
  • all possible modes are stored on class as static L.DistortableImage.Edit.MODES, but this.getModes() will only reflect the specific modes permitted on the image.
  • adding or removing tools will also add or remove the corresponding modes
  • Fixed doubleTap leaflet / chrome bug to allow ability to doubleTap images to change modes on touch screens
  • Fixed image deselection after touchmove bug on touchscreens
  • Make a collection interface animation with a Mutation Observer in handles/AnimateHandle.js
  • updated UI and z-index settings - images are now all in their own div containers that determine their z-index. Containers can have 3 diff classes appended to determine level of z-index for image compared to others
    • bringToFront and bringToBack will no longer do anything bc of the containers
  • lock mode tooltip
  • updated tests to use new MouseEvent for synthetic events instead of deprecated initMouseEvents / createEvent and added a polyfill for Phantom JS browser to run this ok
  • nextMode (double click to iterate through modes) will only iterate through selected modes, and setMode will return false if you try to set a non-selected mode.
  • move L.UnlocksActions into its own file like all other actions and rename L.UnlockAction

pending:

  • make sure animations are all gone on image removal or image.editing.disable
  • added polyfill for web animations API. confirm using it as efficiently as possible. Create a fallback option to just use old UI.
  • check status of L.StackAction given above
  • fix test
    // it('Returns false if image is not selected', function() {
    // expect(overlay.deselect()).to.be.ok;
    // expect(overlay.deselect()).to.be.false;
    // });
  • add an option for disabling single interface UI if the user doesn't want outlines and stuff.
  • rebase grab drag mode.
  • locked images animate in collection mode or no? If not clean up that from MutationObserver
  • fix if distorting a selected image and its handle overlaps another one that one gets highlighted for a sec. _moving property does not account for this

@sashadev-sky
Copy link
Member Author

@jywarren This PR is all about tieing together big picture loose ends LDI.
The modes API: toolbar actions can be modes, explicitly limit the actions array or use removeTool, etc. API to limit / add modes. (I say explicitly because this relationship isn't organic - you'll have all the modes available even if you pass suppressToolbar and have no toolbar at all. The collection class can also set a mode on an instance that doesn't have that mode. If all the instances in a collection share a common mode - that will be the collection's mode and its toolbar tool will highlight (currently just 'lock' and the absence of lock)

@sashadev-sky
Copy link
Member Author

code is implemented and "done" as in bugfree, documented, tested but I need to circle back around and refactor the very complex chains of logic. I think what will be key is factoring out all UI logic somewhere else.

I created a Mutation Observer extending L.Class and moved animation related code in there. This came out a lot cleaner and I am thinking to try to move more code in there. What are thoughts?
Pretty sure each image should not have its own observer so need to change that right? I have never used this API before and just learned about it.

Currently there is a layerGroup of Mutation Observers saved on the featureGroup. I couldn't think how else to add it, making modules without modules is tough!

@sashadev-sky
Copy link
Member Author

And the updated UI: - I think fixes anything left that was confusing like lock. lmk!
new

@sashadev-sky
Copy link
Member Author

i'm imagining Mutation Observer to be like a "single source of truth" situation - top-down UI updates optimized in batches - like React but without the virtual DOM. But I am not sure i understand it practically bc then why isn't everyone using it

@sashadev-sky sashadev-sky mentioned this pull request Dec 22, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant