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

BoxControl: Add support for grouped directions (vertical and horizontal controls) #32610

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Jun 11, 2021

Description

Related to (and potential use case in): #32366

This PR adds a splitOnAxis prop to the BoxControl component, allowing grouped vertical and horizontal controls. The use case for this is for Gap support, where top/bottom and left/right values need to be grouped together to be used in the row-gap and column-gap CSS properties, respectively.

The behaviour in this PR is that with splitOnAxis set to true, when unlinking the box control, two unit controls will be displayed for the user to work with, and they'll be displayed inline, instead of on a following row.

How has this been tested?

  1. Check out this PR, and start up Storybook with npm run storybook:dev
  2. Open up Storybook in your browser and navigate to http://localhost:50240/?path=/story/components-boxcontrol--grouped-directions to test out this variation of the BoxControl
  3. To test out on a block within the block editor, switch the default value of splitOnAxis to true on this line: https://github.com/andrewserong/gutenberg/blob/43c68cb18cbdaf712c183d539efdd5e07c4d5099/packages/components/src/box-control/index.js#L57-L57
  4. To run the JS tests locally, use npm run test-unit -- packages/components/src/box-control/test/

Then, in the post editor, add a Group block, and test out using the padding spacing control. It should work as expected updating the padding vertically and horizontally when unlinked.


A question for anyone reviewing: do the property names make sense? Can you think of a better name than isGroupedDirections? Sara gave the suggestion to use a name like splitOnAxis 👍

Screenshots

box-control-grouped-directions

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.
  • I've tested my changes with keyboard and screen readers. (tested via keyboard)
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@youknowriad youknowriad added [Feature] UI Components Impacts or related to the UI component system [Type] New API New API to be used by plugin developers or package users. labels Jun 11, 2021
@jasmussen
Copy link
Contributor

This is cool. This is what I see when I manually enable it for the padding control:

grouped

As a user experience. it's cool. But I do also think that an important next step is figuring out the best use-case. As was the genesis of this PR, it's made for and perfect for gap, which is either a single uniform gap, or either vertical/horizontal based. The control would make complete sense to use for that.

As nice as it is for padding, though — I do like the simplicity of either a uniform padding for all 4 directions, or individual control over either. Having a step in between, or having inconsistent behavior across blocks. So I'm not sure we should surface it as an option for that control at all. Or am I missing additional use cases?

Thanks again, this is so cool, and I really hope we can get some "gap" going. Galleries, navigation, buttons, social links, columns — they'd all benefit substantially!

@andrewserong
Copy link
Contributor Author

Thanks for testing this out, Joen!

So I'm not sure we should surface it as an option for that control at all. Or am I missing additional use cases?

Yes, I was imagining we'd initially just use this for Gap support, to keep things simple for the existing Padding and Margin controls. Happy to explore other options if folks can think of a clear use case, but I agree, it could be awkward trying to introduce it as a step between the "all" control and individual side controls.

For the purposes of this PR, the scope I'm aiming for is to add in the additional feature with it switched off, and we'd then explore switching it on in a separate PR that implements the Gap support feature (e.g. in #32571 if that approach looks viable). We can always clarify the intent in the docs, too, that this option is designed for dealing with properties (like gap) that don't support separate top/bottom or left/right values (though I can't think of any other obvious examples off the top of my head).

Thanks again, this is so cool, and I really hope we can get some "gap" going.

I'm pretty excited about the possibilities, too. It'll be fun seeing which blocks will benefit from Gap support! 🙂

@@ -105,6 +105,14 @@ If this property is true, a button to reset the box control is rendered.
- Required: No
- Default: `true`

### isGroupedDirections
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For this to be more semantic english, should we use has instead of is here given directions is plural?

An even more descriptive prop name might be like splitOnAxis or something like that. Grouped directions could mean corners are grouped together for example.

Suggested change
### isGroupedDirections
### hasGroupedDirections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I love some good naming tips, the is never sit right with the plural for me. I actually quite like splitOnAxis, so I've updated this PR to use that instead. Happy to rename again if you think there's a better option.

@andrewserong
Copy link
Contributor Author

Thanks for the thorough reviews @sarayourfriend and @ciampo, much appreciated! 🙇

I've gone in and made most of the suggested changes, and left a comment on one of the others. I also added a couple of simple tests for the unlinking behaviour, just to get a bit more coverage there. Let me know if there are any other changes you'd like to see, and I can take a look tomorrow 🙂

@andrewserong
Copy link
Contributor Author

Thanks again for the reviews, everyone! I've implemented all the feedback, so I think this is ready for a final review now if you get a chance @ciampo or @sarayourfriend, but do let me know if you'd like to see any other changes made 🙂

@ciampo ciampo self-requested a review June 16, 2021 09:51
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Works well as per testing instructions! 🚀

Also noting some e2e failures. I triggered a re-run, let's see if they still fail.

@glendaviesnz glendaviesnz merged commit f1f0cd7 into WordPress:trunk Jun 16, 2021
@github-actions github-actions bot added this to the Gutenberg 10.9 milestone Jun 16, 2021

const groupedSides = [ 'vertical', 'horizontal' ];

export default function VerticalHorizontalInputControls( {
Copy link
Member

Choose a reason for hiding this comment

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

It seems we could call this AxialInputControl to be more succinct and clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I like that name! Thanks for the suggestion, I can put up a small PR to rename it 👍

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks!

@andrewserong
Copy link
Contributor Author

I've added a follow-up PR to rename VerticalHorizontalInputControls to AxialInputControls based on Matias' suggestion: #33016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants