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

Snapping Guidelines #3060

Merged
merged 110 commits into from
Oct 7, 2019
Merged

Snapping Guidelines #3060

merged 110 commits into from
Oct 7, 2019

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Aug 21, 2019

Summary

Fixes #2992 (resizing blocks in relation to the page).
Fixes #2994 (resizing blocks in relation to other blocks).
Fixes #2993 (moving blocks in relation to the page).
Fixes #2995 (moving blocks in relation to other blocks).

As previously discussed, this PR addresses the snapping guidelines part, but not currently the actual snapping.

Resizing

  • What if the resizing block is rotated?
  • What if a block other than the resizing block (aka one of its siblings) is rotated?
  • Clear snap lines when too far away from snap line? (would trigger another state change)
  • Allow disabling snapping when dragging, but still shown helper lines
  • Add snapping points for vertical and horizontal center too?
  • Snap line is flickering quite often
  • Address block multi-selection interference see Resizing causes multiselection #3092.

Dragging

  • What if the dragging block is rotated?
  • What if a block other than the dragging block (aka one of its siblings) is rotated?
  • Clear snap lines when too far away from snap line? (would trigger another state change)
  • Make proof-of-concept implementation usable
  • Allow disabling snapping when dragging, but still shown helper lines
  • Prevent block from being stuck in snapping position

General

  • Tidy up code
  • Allow disabling snapping when rotating (unrelated, but seemed worthwhile)

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 21, 2019
@swissspidy swissspidy force-pushed the add/2875-snapping branch 4 times, most recently from dfce070 to a7c2c0e Compare August 21, 2019 17:35
@swissspidy

This comment has been minimized.

@swissspidy

This comment has been minimized.

@swissspidy swissspidy marked this pull request as ready for review August 23, 2019 12:28
Copy link
Contributor

@barklund barklund left a comment

Choose a reason for hiding this comment

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

This is great. I approve.

But is there an "implement actual snapping" ticket to follow this one? I can't find one on either board. Isn't this a high priority for 1.4?

@miina
Copy link
Contributor

miina commented Oct 7, 2019

Two notes from functional testing, both can be for future consideration if not blockers right now.

  1. In some cases the snapping lines feel a little bit confusing, that's especially the case if the lines are shown related to another block which doesn't have a background color, for example:

Screenshot 2019-10-07 at 18 43 18

In this case the Title block is quite large but has white background and it's quite confusing to understand what are the snapping lines for. Perhaps it would be a good idea to display the border around the block that's being snapped against?

  1. When the background is red or some other similar color then the snapping lines aren't very visible and strain the eye a bit. AI could be helpful here perhaps.

@swissspidy
Copy link
Collaborator Author

@barklund I'll create one after merging this. We need to flesh out the acceptance criteria there carefully as the last version in this PR was not 100% satisfying.

@miina Some very good points! We should definitely follow up on these.

For the first one, Morten mentioned something similar in #3060 (comment)::

I can't see which element I'm snapping to. It's often not clear if a snap line is related to an object or e.g. the page center. This is done in different ways in other programs - e.g. by only having the snap lines extend to far edge of the object you're snapping to - so only if it's the page itself, the line will be full-height. Another option is to highlight the "snapped" element with a border or slight box shadow. This latter issue is of course made even more complex by the right one above.

I implemented the first idea (only having the snap lines extend to far edge of the object you're snapping to), but not the second one with highlighting the snapped element. I don't know any program that does this (just like adapting the snap line color), so it felt premature to add it.

@miina
Copy link
Contributor

miina commented Oct 7, 2019

Hmm, actually, seeing some other oddities, could you confirm if you also see this?

  1. It seems to start lagging after adding a few blocks, and generally behaves so that the usability is not the best, it became not possible to drag.
    dragging

  2. CTA block is showing tons of lines and it's quite hard to understand which line is the correct line for centering, maybe for the CTA block only the Page limits + the centering line would make sense for now? Perhaps other blocks are not that relevant?
    cta-lines

@barklund
Copy link
Contributor

barklund commented Oct 7, 2019

  1. It seems to start lagging after adding a few blocks, and generally behaves so that the usability is not the best, it became not possible to drag.

I have seen that for a while actually. It's in develop too and have been for about a week - I haven't tracked it back to where it originally comes from.

But when there's multiple elements on the page, you can't select-and-drag in one click (not always at least). You have to click to select first, then drag.

I wasn't sure if it was a bug locally for me only as nobody else reported it though, but it seems it may not be.

  1. CTA block is showing tons of lines and it's quite hard to understand which line is the correct line for centering, maybe for the CTA block only the Page limits + the centering line would make sense for now? Perhaps other blocks are not that relevant?

Hm, the opacity definitely makes it harder to see what's being snapped to at least. Tough one. UX?

@swissspidy
Copy link
Collaborator Author

Created #3458 for the lagging issue.

As for the CTA block: could we change it so that the lines are shown on top? Definitely shouldn't be hidden like that. Regarding the amount of snap lines it would be good to get some real world testing I think. It's not just for the CTA block where we need to find a good balance.

@swissspidy swissspidy merged commit 67e8238 into develop Oct 7, 2019
@swissspidy swissspidy deleted the add/2875-snapping branch October 7, 2019 22:40
@swissspidy swissspidy added this to the v1.3.1 milestone Oct 7, 2019
@swissspidy swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
5 participants