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

Add `DimensionControl` component #16791

Open
wants to merge 6 commits into
base: master
from

Conversation

@getdave
Copy link
Contributor

commented Jul 29, 2019

Screen Shot 2019-07-29 at 13 59 53

Sub PR of Adds basic dimension controls to Group Block.

Adds a standardised <DimensionControl> component which provides a UI for changing dimension/spacing values.

  • Standardised, accessible markup
  • Customisable label
  • Optional customisable icon

Please note

This isn't in any way wired up to Block state so changing the values doesn't do anything. This is purely a PR for the UI only.

How has this been tested?

I've temporarily hacked and implementation into the core/group Block which utilises <select> inputs and temporary state for demo purposes.

Note that this will be removed before merging.

Screenshots

See above.

Toggle open

Screen Shot 2019-07-29 at 13 53 11

Types of changes

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.
@karmatosed

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Just going to pop a little design review here, thanks for this.

image

  • The section being called 'spacing' but seeing 'padding' threw me a little.
  • I see here small through to extra-large but it was hard to work out 'what those values' are.
  • I didn't seem to be able to get any changes to the padding despite saving and viewing a few times. It also seemed to get stuck on reload back to default option. This was in Chrome.

@karmatosed karmatosed self-requested a review Aug 2, 2019

@getdave getdave requested a review from shaunandrews Aug 2, 2019

@shaunandrews

This comment has been minimized.

Copy link

commented Aug 2, 2019

My initial thought here was to try using a Range component, which came out like this:

image

However, when considering #16790 and other scenarios where we might have multiple controls listed the Range component gets really messy and distracting.

image

--

So I tried sticking with the dropdown but removing the Icon and Label elements:

image

This is simpler overall, and lends itself better to showing multiple controls in a row.

--

I notice you have a default value of "Please select...". I find this placeholder value confusing. Instead, perhaps we add a "Default" value:

image

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Thanks for breaking this out into a smaller issue, @getdave.

A base concern I have about this change is: How can we make this manageable for themes? On a basic level, we'll want themes to be able to specify what exactly these sizes mean. (In the near term at least). Similar to how themes can provide custom font sizes.

I'm relatively certain that the main point of difficulty from theme development is the fact that any margins that are added here will need to be overwritten by themes for any full-width innerblocks. We already have a little taste of what this is like based on the simple 30px padding added by default to Group blocks with a background color. The Gutenberg Starter theme manually adjusts the width, max-width, and position of full-width inner blocks to override that:

https://github.com/WordPress/gutenberg-starter-theme/blob/043fb28c64ee370551d4df1ac3611c4b4385604e/css/blocks.css#L136-L141

Twenty Nineteen handles this similarly. It rewrites that 30px to align its base styles, and then it adjusts the width, max-width, and margin of full-width blocks inside it to match.

To get this working with custom sizes, themes will need to essentially write in that same change, but for all supported custom spacing sizes.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@shaunandrews Thanks for your feedback and insight. It is much appreciated.

sticking with the dropdown but removing the Icon and Label elements:

I like the approach of removing the icon for the simple controls (ie: when you don't have "responsive" controls. I think the icon is probably necessary for the responsive mode but icon can be an optional prop on the component so we're all good.

From an a11y perspective removing a visual label could be problematic. I also feel it might affect UX. The mockup you seem to have landed on still shows a label which suggests you also came to this conclusion. Can you confirm?

I notice you have a default value of "Please select...". I find this placeholder value confusing. Instead, perhaps we add a "Default" value:

You are right. Please select... isn't clear that it is basically saying "revert to default setting". I'll try your suggestion.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@kjellr Thanks for your insightful feedback on Theme issues.

Thanks for breaking this out into a smaller issue, @getdave.

No problem!

On a basic level, we'll want themes to be able to specify what exactly these sizes mean. (In the near term at least). Similar to how themes can provide custom font sizes.

I agree. I was hunting in the docs to see if this was already a thing but it isn't and I think it should be. How can we make this happen? What's the impact for Themes? Can you help?

I'm relatively certain that the main point of difficulty from theme development is the fact that any margins that are added here will need to be overwritten by themes for any full-width innerblocks.

Yes that's true. I remember what a minefield writing that CSS was (if you recall)!

I'm keen to discover what the true scale of problem is we looking at? Is it huge or manageable?

From my understanding, our options seem to be the following:

  1. We add some logic (external to the component itself as this is not the concern of the component itself) which disables the UI for (and impact of) margin for Group blocks which are set to full-width alignment.

  2. We add support to the Gutenberg and Twenty Nineteen themes to support the S, M, L, XL sizes we define.

  3. We produce docs advising Theme devs how to make the necessary CSS changes.

My questions are:

  • which of the above options are desirable?
  • which of the above options are feasible?
  • which of the above options are required for this PR to land?
  • are any of the above options insurmountable (if so then I'd rather can this right now 😄 )?

Much appreciated

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

which of the above options are desirable?
which of the above options are feasible?
which of the above options are required for this PR to land?
are any of the above options insurmountable (if so then I'd rather can this right now 😄 )?

Good questions! I definitely don't think any of this is insurmountable! So that's the good news. 😄 I'll let others speak to the feasibility, but I can try to outline what I think are the desirable + required
options.

Desirable

I think the ideal state would be:

  1. Similar to wide/full images, we offer a theme opt-in for custom padding in general. If this is not enabled, these controls don't show up. That way there's basically no chance this will break any theme.
  2. We offer a theme opt-in for themes to provide their own custom sizes. This is the equivalent of the custom font size options Gutenberg already provides.
  3. We update the default themes to support both of those.
  4. We write up documentation for theme developers to adopt both.

We could theoretically make 1 & 2 variations of the same opt-in. So a basic opt-in would be:

add_theme_support( 'editor-padding-sizes' );

or, you could add an optional array to specify custom sizes:

add_theme_support( 'editor-padding-sizes', array(
    array(
        'name' => __( 'Small', 'themeLangDomain' ),
        'size' => 8px,
        'slug' => 'small'
    ),
    array(
        'name' => __( 'Normal', 'themeLangDomain' ),
        'size' => 16,
        'slug' => 'medium'
    ),
    array(
        'name' => __( 'Large', 'themeLangDomain' ),
        'size' => 24,
        'slug' => 'large'
    ),
    array(
        'name' => __( 'Huge', 'themeLangDomain' ),
        'size' => 36,
        'slug' => 'extra-large'
    )
) );

Themes are opted into custom font sizes whether or not they declare theme_support, so this works a little differently... but it's cleaner than forcing themes to do two opt-ins.

(Aside: It would be great to allow themes to specify values other than px here, but I know that's all we allow for the font-size one currently).

Required

The main "requirement" for this to land is relatively simple: that it doesn't break many themes out of the box. 😄 But I'll go through that list above:

  1. Similar to wide/full images, we offer a theme opt-in for custom padding in general. If this is not enabled, these controls don't show up. That way there's basically no chance this will break any theme.

This is (probably?) not a requirement. I think we can eliminate the general theme-opt-in by adopting something like you mention above — the logic that disables the UI and impact of margins for full-width group blocks. This is somewhat unideal though, so we may need to add some sort of language in the UI to explain what's going on.

Alternatively: the left/right padding is what causes the problems here, so another way to handle this would be to ignore that in the case of full blocks, and have them only use the top/bottom padding. Maybe by default, those are just ignored for full-width blocks? Again, this is not totally ideal, and would likely require some explainer text.

  1. We offer a theme opt-in for themes to provide their own custom sizes. This is the equivalent of the custom font size options Gutenberg already provides.

Assuming we figure that first part out, the theme-supplied custom sizes are something that should launch with this feature. But I'm not totally sure I'd say it's a requirement. It definitely needs to happen, but maybe not in this exact PR.

  1. We update the default themes to support both of those.

Whatever we end up doing for the first two points, the default themes will need to need to be tested and updated to support this feature. That's definitely a requirement.

  1. We write up documentation for theme developers to adopt both.

If this does require any themes to update, then the documentation will obviously have to be updated alongside this PR. So I'd say that is a requirement.


I think that covers everything, but I'd love some feedback from other theme developers as well.

@allancole

This comment has been minimized.

Copy link

commented Aug 12, 2019

A base concern I have about this change is: How can we make this manageable for themes? On a basic level, we'll want themes to be able to specify what exactly these sizes mean. (In the near term at least). Similar to how themes can provide custom font sizes.

Spent a little time digesting this and I think I have a slightly different opinion on this. I feel like themes should probably to stay out of the way of these per-block style settings. If anything, themes should come with one standard default spacing value and then if a customer changes the spacing in the editor, the theme should just respect the override.

I'm relatively certain that the main point of difficulty from theme development is the fact that any margins that are added here will need to be overwritten by themes for any full-width innerblocks.

Yeah, this is a super tricky condition to account for. My feeling on this is that spacing controls should only have an inward effect on blocks (example). Outer margins should not be effected so we can maintain the behavior in .alignwide, .alignfull and other utility classes. If users want more space between two blocks vertically, the Spacer Block is the probably a better method, I think. In that way, this tool might only need to effect padding.

Similar to wide/full images, we offer a theme opt-in for custom padding in general. If this is not enabled, these controls don't show up. That way there's basically no chance this will break any theme.

If these are rolled out with utility classes, we might not need to worry about the theme breaking at all. I don’t see something like .has-large-top-padding { padding-top: 24px } breaking any theme. CoBlocks and Kioken do a similar thing that’s worth experimenting with if you haven’t already. Both rely on plugin-based utility classes that essentially can work with any theme that doesn’t entirely overwrite Gutenberg the styles.

We offer a theme opt-in for themes to provide their own custom sizes. This is the equivalent of the custom font size options Gutenberg already provides.

Assuming we figure that first part out, the theme-supplied custom sizes are something that should launch with this feature. But I'm not totally sure I'd say it's a requirement. It definitely needs to happen, but maybe not in this exact PR.

Yeah, I kind of feel the same about this. I’m not sure it needs to happen just yet. Even if themes come with a predefined set of values, customers should still be able to choose their own value (just like font-sizes). Because of this, I think this could launch without theme specific values, as the arbitrary values will still be useful.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@kjellr Thanks again for your feedback here. Much appreciated.

Good questions! I definitely don't think any of this is insurmountable! So that's the good news. 😄

That is good news 😄

We could theoretically make 1 & 2 variations of the same opt-in

Custom spacing should be opt-in. However, I think most users would expect the ability to change padding/margin to be part of a default base experience. Therefore I'm not sure we should ask Themes to opt in to allowing the spacing controls to show up.

It would be great to allow themes to specify values other than px here

That's probably possible. I'm not clear as to why font sizes only allows for px. Is there a reason?

The main "requirement" for this to land is relatively simple: that it doesn't break many themes out of the box.

Agreed.

I think we can eliminate the general theme-opt-in by adopting something like you mention above — the logic that disables the UI and impact of margins for full-width group blocks. This is somewhat unideal though, so we may need to add some sort of language in the UI to explain what's going on.

I think (as suggested above) this is a good approach. We might need some explanation text yes.

Assuming we figure that first part out, the theme-supplied custom sizes are something that should launch with this feature. But I'm not totally sure I'd say it's a requirement.

I'd say it's a separate PR. We land with sensible defaults and then add another PR for custom spacing. Once that's in we integrate into the work in this PR.

...the default themes will need to need to be tested and updated to support this feature. T

Agreed. A requirement that is outside of this PR but will need to be completed before we hit "Merged" on this one. Who could help with that? Yourself?

...the documentation will obviously have to be updated alongside this PR. So I'd say that is a requirement.

Yep a docs update is required.

Thanks so much for your feedback. Looks like @allancole has left more which I'll respond to now.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@allancole Thanks for your feedback. Sorry it's taken so long to come back you!

If anything, themes should come with one standard default spacing value and then if a customer changes the spacing in the editor, the theme should just respect the override.

I'd agree that once we have the ability to manually define values then the user selection within the Editor should overide any default of Theme-specific settings. However, this PR doesn't reach that stage. All we do is add classes to the DOM elements in question and the editor/theme can/could choose to apply whatever styling they like to these to deliver the correct spacing. This is why I've gone for arbitrary small, medium, large...etc. It doesn't bind us to explicit sizes and provides a good balance of flexibility whilst preserving simplicity.

My feeling on this is that spacing controls should only have an inward effect on blocks

I'm happy with that if that's what your experience as a Theme developer tells us. This PR just provides the controls. We can choose to make the control for padding or margin or both as required. If it's easier to ignore margin for now and that helps us land this PR then so much the better. Do you agree @kjellr ?

If these are rolled out with utility classes, we might not need to worry about the theme breaking at all. I don’t see something like .has-large-top-padding { padding-top: 24px } breaking any theme.

They will if these styles are included in the editor default styles on a per Block basis. The parent PR has an example of this. Without providing defaults this functionality won't work unless the Theme supports it. As I indicated above in response to @kjellr I feel that users would expect this to work "out of the box" across all Themes. I'm very open to an alternative perspective on this however.

Even if themes come with a predefined set of values, customers should still be able to choose their own value (just like font-sizes). Because of this, I think this could launch without theme-specific values, as the arbitrary values will still be useful.

I didn't entirely follow. To be clear I"m against introducing manually customizable values in the Editor at this stage (ie: where you could tweak each value in px directly in the Editor).

@karmatosed

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I have to echo other sentiments here that how this plays out across the wide range of themes is worrisome. There is prior art in theme_support but it's what we decide on the default to be.

@kjellr has a strong point about the ideal state. I agree that the way full/wide images has been done stands us in a good place for this. I would add noting this new change in dev docs is key for release. It could fit nicely into WordPress 5.3.

My vote is for that as doesn't break anything out of the box.

I would +1 ignoring padding when the block is full width. It isn't ideal but it seems like the best default.

@allancole

This comment has been minimized.

Copy link

commented Aug 14, 2019

It doesn't bind us to explicit sizes and provides a good balance of flexibility whilst preserving simplicity.

Okay, that’s great to know. I’m probably thinking a bit too far ahead, but as long as there’s a good amount of flexibility, I’m on board :-)

This PR just provides the controls. We can choose to make the control for padding or margin or both as required. If it's easier to ignore margin for now and that helps us land this PR then so much the better.

As I think on this more, I can definitely see situations where I would want to have both. Spacer block is good for adding extra vertical space between blocks, but sometimes I want to remove space and to do that, we’d need a feature like what you’ve suggested here.

I don’t see something like .has-large-top-padding { padding-top: 24px } breaking any theme.

They will if these styles are included in the editor default styles on a per Block basis.

I’m sure I’m probably missing some context here, but do these styles need to be added on a per-block basis? I’m imagining myself using this on any block where I might want to arbitrarily alter the spacing in some way. Seems like all blocks—nested or otherwise—could benefit from this kind of spacing flexibility so utility classes seems like an easier method to me.

I’ll also say there’s quite a few plugins that offer this same functionality. The two I’m thinking of: CoBlocks and Kioken, both rely on utility classes and have (almost) zero conflicts with theme styles. That’s definitely influencing my expectation here, but its very possible I’m missing the context that makes per-block styles more appropriate.

To be clear I"m against introducing manually customizable values in the Editor at this stage (ie: where you could tweak each value in px directly in the Editor).

Sure, I’m okay with that not making it into this stage. 👍

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