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

ResizableBox: Add visual handles for side resizers #14543

Merged
merged 10 commits into from Apr 12, 2019

Conversation

@marekhrabe
Copy link
Contributor

commented Mar 21, 2019

Description

This builds on top of my #14481 which made handles much bigger. This PR adds a secondary visual aid to them, apart from the mouse cursor.

The blue line design was suggested in #14410 (comment) by @melchoyce. In this PR, it differs a tiny bit from the sketch as it has an extra white border around the blue circle. Without changing the markup to add more elements, this is the best we could do I think.

I've made them appear only on hover in the first iteration - sliding out of the blue circle to indicate what control they belong to.

How has this been tested?

Test with blocks: Media & Text, Image (align it "center" to show most handles at once) and Spacer. They all use ResizableBox internally.

Screenshots

visual-resizers

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

marekhrabe added some commits Mar 18, 2019

@marekhrabe marekhrabe self-assigned this Mar 21, 2019

@marekhrabe marekhrabe requested a review from melchoyce Mar 21, 2019

@marekhrabe marekhrabe requested a review from chrisvanpatten as a code owner Mar 21, 2019

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@melchoyce What do you think?

@mapk

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

I really like the way this is looking, @marekhrabe! Thanks for working through this.

In the image below, I circled three different line weights which bothered me a bit. There's a really thin 1px block outline, a 3px left block border, and then the 2px blue resizable bar.

weights

In an attempt to add a bit of consistency, I made the resizable blue border thicker in the example below to match the weight of the left block border. This feels a little heavy, so I'd like some feedback from other designers. @melchoyce @kjellr thoughts?

lines

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Also the blue dot and also the blue line both have a white outline of 2px to distinguish them from content. Would that be something that we should focus on too?

Just a tiny technical one: we cannot center the blue line properly towards the blue dot, unless we either make it even one more pixel wider than you did or we make the dot one pixel smaller.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Also the blue dot and also the blue line both have a white outline of 2px to distinguish them from content. Would that be something that we should focus on too?

Yeah, we need to adjust this a bit so that it works on dark themes. Currently it's pretty jarring:

Screen Shot 2019-03-27 at 8 11 18 AM

A couple options:

  • Remove the white entirely. This makes the blue harder to see, but I don't think it's necessarily a bad option.
  • Replace the white with one of our opacity values. If we want with this route, we'd want to also build in a .is-dark-theme & style, like we do for block borders.

Just a tiny technical one: we cannot center the blue line properly towards the blue dot, unless we either make it even one more pixel wider than you did or we make the dot one pixel smaller.

I'd like to see these match up nicely, so let's try making the dot one pixel smaller.

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Hmmm, opacity value would probably clash in a weird way where the line and dot meets. I can try it without those border lines it but I remember it used to "disappear" in the image.

@melchoyce

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

CoBlocks doesn't do anything for dark themes, looks like, but it works a little better because all of the lines are thicker:

image

marekhrabe added some commits Apr 8, 2019

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

New thickness and using a light gray instead of full white when dark theme detected (same as the selected block border in fact). Let me know what this iteration looks like

Screenshot 2019-04-08 at 21 48 50

Screenshot 2019-04-08 at 21 46 54

@tomjn

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

The circle doesn't make clear that this is for resizing, which isn't a problem with the mouse cursor, but it could be confusing for users of touch devices?

Perhaps when the control is being moved, it can swap the circle out for arrows

@mapk

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I tested this today and it feels pretty comfortable. The line weights were great.

Perhaps when the control is being moved, it can swap the circle out for arrows

Adding anything more than simple dots might get too overwhelming, especially on mobile. I took a look at Google Docs and they just use simple squares. There are more of them but I think we're at a good position here. Ultimately I'd like to see us work toward the resizing while displaying values like in CoBlocks, but that can be another PR.

IMG_962CAB310976-1

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Ultimately I'd like to see us work toward the resizing while displaying values like in CoBlocks, but that can be another PR.

Absolutely! I'm trying to do granular improvements with my PRs instead of one big. General ideas tracked in #14410

Would you say this is ready to go from a design perspective and I can ask for a final code review?

@marekhrabe marekhrabe requested a review from youknowriad Apr 10, 2019

@mapk

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Would you say this is ready to go from a design perspective and I can ask for a final code review?

From a design perspective, yes. It's ready to go. 👍

// Hide side handle line by default
.components-resizable-box__side-handle.components-resizable-box__handle-top::before,
.components-resizable-box__side-handle.components-resizable-box__handle-bottom::before {
transform: scaleX(0);

This comment has been minimized.

Copy link
@kjellr

kjellr Apr 10, 2019

Contributor

A couple animation notes: Just noting that scale is one of the less performant animations. I don't think it's a huge issue here since we're only animating borders basically. Shouldn't have a huge impact.

In any case though, we should move these into an animation property and combine it with opacity animation as well so this animation relates a bit more to some of the other animations we have happening in the UI. Then we can also loop in the @reduce-motion mixin for users that want to opt out of the animation as well.

@marekhrabe I have that change ready locally — do you mind if I push it to this branch?

This comment has been minimized.

Copy link
@marekhrabe

marekhrabe Apr 11, 2019

Author Contributor

I have that change ready locally — do you mind if I push it to this branch?

You rock! Please do 🤘

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

I had an issue when resizing the media & text image when it's full-width aligned. I don't really think it's related to this PR but worth mentioning. There's a flickering effect and sometimes the image appears cropped or disappear entirely.

Capture d’écran 2019-04-11 à 8 40 19 AM

@youknowriad
Copy link
Contributor

left a comment

Nice work here 👍

I'll leave the final word for @kjellr to add his commit if needed.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Alright, I merged those animation changes in. I had to sync up with master since this PR was started before the reduce-motion mixin existed. I think this needs to be rebased before merged in, but otherwise it should be all set. 👍

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@kjellr I think something went wrong here with the rebase/merge?

@marekhrabe marekhrabe changed the base branch from update/resizable-box-handles to master Apr 11, 2019

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I've updated the PR to target master instead of my other branch which got merged in #14481. All looking good now including the rebase @kjellr done

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Nice let's ship it

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

🎉 Excellent, thanks folks!

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

It's been a pleasure collaborating on this! 🥳

How do I proceed? I cannot merge, as Travis CI is pending and doesn't seem to do anything or provide a way to trigger it.

@youknowriad youknowriad merged commit c82cdcf into master Apr 12, 2019

@youknowriad youknowriad deleted the update/resizable-box-visual-handles branch Apr 12, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Merged, I don't expect this to break any tests, but let's keep an eye on the builds in master.

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

After using this in the wild, I've noticed that the conversion of my transition into the animation has caused additional flickering of the blue line which wasn't present before.

It likely happens because we have styles for both :hover and :active (which we need), however compared to the transition, the animation will is played even when switching from :active back to :hover. That happens when you release the mouse button after a resize.

line-flicker

What was the reason for moving this into animation? Would you be opposed of going back to the transition and implementing opacity and reduced motion for it instead of animation? Unless I'm missing something crucial, it should be an easy change.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Ah, good catch. We could re-adopt transition there if this is causing issues. The main reason for switching to animation was to hook into the existing reduce motion mixin, but I think we'll probably end up adjusting that mixin to account for transition properties too. So it should be fine to change that back.

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Cool, I'll spin up a PR and update the mixin (or figure out another solution). I'll ping you there :)

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Thank you!

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

I've brought back the transition and it solved the issue mostly but some kind of tiny flicker still appeared in limited cases and I haven't been able to figure out how to get rid of it fully.

With that, I'll leave this like it is for now and potentially later investigate the underlying library. I think that the problem lies somewhere there…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.