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: Make invisible resize handles bigger #14481

Merged
merged 3 commits into from
Mar 27, 2019

Conversation

marekhrabe
Copy link
Contributor

@marekhrabe marekhrabe commented Mar 18, 2019

Description

I've suggested several resizing improvements in #14410 and this is one of them. It makes resize handles of ResizableBox much bigger - spanning across the whole side they control instead of just the small blue dot in the center.

I've built this with Media & Text in mind but it seems it helps other blocks too, especially Spacer where the blue dot collides with "Add Block" button.

Known limitation: This bigger handler is only implemented for Left, Right and Bottom resizers. Those are the only resizers that have been supported and used in our codebase so I've continued with the same limitation.

All sides and corners are now supported. Additional testing instructions related to them are in my comments on this PR.

How has this been tested?

Visually, all blocks should look the same and show the blue dot in the center of resizable areas. What should change is the area that you can initiate the resize action from.

ResizableBox component is used in Media & Text, Spacer and Image blocks. Test all of them and confirm their drag & drop resizing still works in all expected cases. Try resizing by dragging anywhere on a side but outside of the blue dot. Make sure the action is appropriately indicated with a resizing cursor.

Screenshots

Green color is for demonstration purposes only to show where the active area is. For users, this should be pretty intuitive as the whole side of media acts as a handler.

resizers-v2

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 marekhrabe added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Block] Image Affects the Image Block [Block] Media & Text Affects the Media & Text Block [Block] Spacer Affects the Spacer Block labels Mar 18, 2019
@marekhrabe marekhrabe marked this pull request as ready for review March 18, 2019 05:42
@mcsf mcsf requested a review from jasmussen March 18, 2019 09:12
@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Mar 18, 2019

This definitely seems like a nice change!

How does it work for the overlapping corners? (I don't have access to a Gute dev environment at the moment) Is it just based on whichever z-index is higher?

One thing to note is that ResizableBox should support draggable corners too (top-left, top-right, bottom-left, bottom-right). I don't believe it's used in any core blocks, but it's still an option. How does this work for those cases?

@marekhrabe
Copy link
Contributor Author

marekhrabe commented Mar 19, 2019

How does it work for the overlapping corners? (I don't have access to a Gute dev environment at the moment) Is it just based on whichever z-index is higher?

Yes, the order of elements in DOM will determine that as I think they don't have distinct z-indexes.

One thing to note is that ResizableBox should support draggable corners too

The underlying library does support it I think but the wrapper component that was made for Gutenberg is only limited for top, bottom, left and right in JS code (no corners) and actually the top is not truly supported either as its CSS class has not been defined.

This keeps us with bottom, left & right as only usable options.

@marekhrabe
Copy link
Contributor Author

We can probably very easily add support to all those possible options so there are no surprises to future users of the component. I think that should be the right way to proceed, considering those options were already advertised in README of that component (despite not working)

@jasmussen
Copy link
Contributor

Judging purely by the goal of this PR, and the screenshots/GIFs (thanks for including those), this is a solid enhancement 👍 👍

I would echo Chris that it'd be nice to explore corner resizing at some point in the future, either through an enhancement to ResizableBox or through finding a better library. But as it is, this PR does not seem to regress or change anything from master in that respact, only enhance, so I think it's fine to take this as an iterative improvement and just keep thinking about the more fundamental improvements separately. Nice work, I'd definitely suggest this is ready for a code review as the next step.

@marekhrabe
Copy link
Contributor Author

marekhrabe commented Mar 19, 2019

I've added support for all handles and made sure corner handles are rendered with higher z-index than sides, so they can actually be used. Our codebase doesn't use those other handles anywhere yet but I think it makes sense to support our underlying library fully so there are no surprises for developers.

When you go all-in (with debug mode):
Screenshot 2019-03-19 at 18 22 33

Without crazy colors:
Screenshot 2019-03-19 at 18 25 53

If you want to test it yourself, you can edit packages/block-library/src/media-text/media-container.js and edit enablePositions const to just include everything:

const enablePositions = {
	top: true,
	bottom: true,
	topLeft: true,
	topRight: true,
	bottomRight: true,
	bottomLeft: true,
	left: true,
	right: true,
};

Crazy colors? Here you go:

.components-resizable-box__side-handle {
	background: green;
}

.components-resizable-box__corner-handle {
	background: hotpink;
}

Then try Media & Text where you should now see all handles. Their drag & drop behavior will not work correctly because Media & Text doesn't expect all those handles however, in this PR it is enough to confirm they are placed correctly against the image, show proper cursors etc… It's up to ResizableBox users to handle additional handles should they choose to have them.

Ready for a review now!

@jasmussen
Copy link
Contributor

Wow, impressive work!

Thanks for making this possible. For now and just to keep this PR smooth sailing, we should probably in the case of the media & text block keep just the one side handle enabled. But good to know we now have additional options!

@marekhrabe
Copy link
Contributor Author

Thanks! Just to make it super clear, I used M&T for easy debugging and I don't expect other handles to be added any time soon.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

I can find any issues here. Seems like a nice enhancement.

I also tested with all the drag handles enabled. Just one note (which you've probably seen) that on Media + Text it feels odd to have the corners on the opposite side (ie: the side furthest from the text) enabled.

Great work!

@getdave getdave added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Mar 19, 2019
@getdave
Copy link
Contributor

getdave commented Mar 26, 2019

I'd say this is good to go 🚀

@aduth
Copy link
Member

aduth commented Mar 27, 2019

Thanks! Just to make it super clear, I used M&T for easy debugging and I don't expect other handles to be added any time soon.

To clarify my own understanding reflecting on the code: The corner resize exists as a feature of ResizableBox, but it is not as of these changes applied to any block. My YAGNI senses are tingling, but I'd allow it 😄

@aduth aduth merged commit d9768ad into master Mar 27, 2019
@marekhrabe marekhrabe deleted the update/resizable-box-handles branch May 28, 2019 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Block] Media & Text Affects the Media & Text Block [Block] Spacer Affects the Spacer Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants