Skip to content

[Layout Engine] Feat: add self-snapping of canvas elements#26092

Merged
monfera merged 1 commit intomasterfrom
self-snap
Nov 28, 2018
Merged

[Layout Engine] Feat: add self-snapping of canvas elements#26092
monfera merged 1 commit intomasterfrom
self-snap

Conversation

@monfera
Copy link
Contributor

@monfera monfera commented Nov 22, 2018

Closes #23978

Summary

Now, when dragging or resizing a Canvas element, its landmark points snap to landmark X/Y values of other shapes. So far, however, the element hasn't snapped to its former self. Reasons for adding self-snapping with this PR:

  1. Comparable software does it too
  2. A main purpose is that an element selection do not introduce an accidental drag of a couple of pixels
  3. It's also good to let users exactly reposition the element to its original place (without relying undo, ie. another way for achieving a rollback)
  4. Often, the user may want to purposefully move the element such that only the X or Y values would change; we'll eventually have a modifier key but it helps to have this means too

As with snapping in general, it can be deactivated with Command on the mac or Alt on the PC (the snap lines still show).

A future PR may add an actual indicator border for the former place of the element. But it'd be useful to merge as is, because every accidental minor drag&drop incurred on element selection incurs network traffic. So this issue partly addresses performance issues reported by @timroes

selfsnap

cc: @ryankeairns @alexfrancoeur

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@monfera monfera added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// label Nov 22, 2018
@monfera monfera self-assigned this Nov 22, 2018
@monfera monfera requested a review from rashidkpc November 22, 2018 15:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@monfera monfera changed the title Feat: add self-snapping of canvas elements [Layout Engine] Feat: add self-snapping of canvas elements Nov 22, 2018
@monfera monfera added the review label Nov 22, 2018
@ryankeairns
Copy link
Contributor

This works as described and is a nice addition.

In other vector editing tools, I've seen the ability to toggle the guides while dragging which is how I expected this to work (i.e. select, drag, then press Command to disable while dragging), whereas here you must press Command before you select/drag. I don't feel strongly enough that I would propose changing it, but perhaps others who review this will have an opinion.

@monfera
Copy link
Contributor Author

monfera commented Nov 26, 2018

@ryankeairns thanks, valid point! I noticed too that it works this way, not just for self-snapping, but for snapping in general, which is, as you say, not as good.

Since this issue is independent of this PR, I'm planning to improve on this via a subsequent, independent PR. In short, making snapping behave the same way eg. the toggle between centered and normal resize works: I'll add it to the description for follow-up work.

I just re-tested, it looks like the snap / no-snap behavior actually gets toggled, but it's only apparent once you then subsequently moved the mouse at least a little bit. The problem is that in JavaScript it's not possible to get a list of what keyboard buttons are down at a given time; I added state for that, but it got removed recently. I'm planning to add it back for this use case but as it's a smaller thing relative to this, and impacts the gesture handling (not the layouting as this PR), the plan is to make it a separate PR.

@alexfrancoeur
Copy link

Beautiful @monfera, I just tried it out. LGTM

@rashidkpc
Copy link
Contributor

Nice change, ship it.

@monfera monfera merged commit a92c649 into master Nov 28, 2018
@monfera monfera deleted the self-snap branch November 28, 2018 08:57
@monfera
Copy link
Contributor Author

monfera commented Nov 28, 2018

@rashidkpc @timroes should it be backported to 6.5 and 6.x?

@rashidkpc
Copy link
Contributor

6.x yes, 6.5 no. 6.5 should be limited to bug fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t//

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants