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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds basic dimension controls to Group Block #16730

Open
wants to merge 24 commits into
base: master
from

Conversation

@getdave
Copy link
Contributor

commented Jul 24, 2019

Update

Following multiple requests, this is being broken up into separate PRs.

  1. Add standardised ResponsiveBlockControl component
  2. Add DimensionControl component

Screen Shot 2019-07-24 at 21 20 09

馃帴 Screencapture showing this in action - https://cld.wthms.co/VRc6R2

Description

Adds dimension controls to the Group Block via a new DimensionControl component within the @wordpress/block-editor package.

Closes #14747

Please note: this PR aims to implement the basic feature set for dimension controls. More complex UI will be added at a later date. For a more detailed explanation see this comment.

Todo

  • Ensure i18n best practices are observed
  • Unit tests
  • Add deprecation to core/group Block
  • Add reset UI
  • Add individual padding side control (probably out of scope)
  • Enable responsive settings

Design/UX questions

  • How can we provide a UI that allows changing the padding value across different devices?

How has this been tested?

Manually for now.

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.

@getdave getdave self-assigned this Jul 24, 2019

Improve i18n of strings.
Addresses #16730 (comment)

Renames `type` to `property` to better indicate that it is a formal CSS property value and therefore should not be translated lest it break the underlying code.

Added title attribute to enable true text-based strings to be translatable.
@paulwilde

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Maybe Extra Large (XL) would make more sense instead of Huge (H). I didn't know what "H" stood for at first.

@getdave getdave requested review from obenland, frontdevde and marekhrabe Jul 24, 2019

@getdave getdave requested a review from kjellr Jul 24, 2019

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@kjellr has pointed me at some nice UI for responsive controls. I'll be implementing that I've implemented those!

#13363 (comment)

@getdave getdave marked this pull request as ready for review Jul 25, 2019

@SchneiderSam

This comment has been minimized.

Copy link

commented Jul 25, 2019

Nice, but I find the information with S, L, M, XL ect somewhat strange. Why not with pixels? The current variant is nice for beginners, but very inaccurate for advanced users...

@drw158

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Based on #13363 (comment)

Some design feedback:

  • The switch help text shouldn't be phrased as a question. Also, I don't think margin and padding should be capitalized in the switch help text.
  • The help text under each of the controls is redundant (Select the padding for this block).
  • The labels could be simplified. Try Mobile instead of Padding (mobile devices).
  • I assume the double arrow icon on the right resets the setting but I'm not sure. Might be better to use a text button that reads "Reset".
  • I don't know what N and X do.
@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I find the information with S, L, M, XL ect somewhat strange.

@SchneiderSam Thanks for your opinion. I assume by "strange" you mean that it wasn't immediately clear to you why the arbitrary values had been selected over absolute units?

Why not with pixels?

Imagine you are a Theme designer. You craft your CSS with spacing that is perfect for the design. You want to ensure that is consistent throughout your Theme, even if the page layout is being created by the end-user in the Block Editor.

With the approach I've taken here, when a size is selected only classes are added to the Block in the DOM. This affords the Theme creator the opportunity to provide custom sizes in CSS that are suitable for their Theme. If they opt not to do this then sensible defaults are provided.

With the pixels approach, we're locking users of the Block into absolute values and asking them to make a lot of decisions that they'd probably prefer not to have to make. It could also lead to an inconsistent visual experience.

The current variant is nice for beginners...

I completely agree. This is why I've selected this approach for the first pass at adding this control. Average users (whoever they are!) will likely be grateful to receive a series of predefined options rather than having to make their own choices. I'm looking to cater to them first, which also serves to keep this PR simple and easily comprehensible, this increasing the changes it will be accepted and merged.

...but very inaccurate for advanced users...

I take your point. As a power-user, you want precise control which this approach doesn't currently provide.

Please consider however, that at this point we're introducing a new feature with a totally new UI. As a result, I'm keen to avoid overcomplicating this PR or increasing its scope.

If this UI ships, in the future, I imagine we will want to enhance it with an "advanced" option which will afford power users the ability to

  • specify absolute values
  • specify the values for each "side" individually (eg: top, left...etc)

Ultimately, there's a lot of things we could look to do. However, for now, I'm hoping you will agree that keeping things simple and focus on the 90% of users will afford us the best opportunity of getting this PR merged. We can iterate from there.

Once again, I want to say how much I really appreciate you taking the time to provide feedback.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@drw158 Thanks for your feedback! All good stuff. I'll look at updating it tomorrow.

I don't know what N and X do.

Note that there are tooltips for each option with the full text available.

I think X has now become XL which should solve things. Do you agree that is clear enough?

I agree that N is a problem. I'd value any suggests for alternatives. Bear in mind an explicit None option is required because some Blocks have default padding and so we need None as an option to allow users to completely remove the padding from the Block.

@paaljoachim

This comment has been minimized.

Copy link

commented Jul 25, 2019

I agree that N is a problem. I'd value any suggests for alternatives. Bear in mind an explicit None option is required because some Blocks have default padding and so we need None as an option to allow users to completely remove the padding from the Block.

What about a No with a tooltip of None.

getdave added 2 commits Jul 26, 2019
Utilise withInstanceId HOC to remove need for id prop
Also updates tests to account for this.
Updates size buttons for improved comprehension and a11y
Updates `N` to be `No`. Also avoids string manipulation approach to getting visual size button label in favour of explicit abbr prop on sizes.
@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

What about a No with a tooltip of None.

I don't know what N and X do.

@drw158 @paaljoachim I've updated to the below (No and XL) and also improved the a11y

Screen Shot 2019-07-26 at 10 32 26

Is this better?

@paaljoachim

This comment has been minimized.

Copy link

commented Jul 26, 2019

I shared the PR in the #design channel on Slack to get some additional thoughts.
@mapk

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@drw158 I've fixed the below based on your feedback:

The switch help text shouldn't be phrased as a question. Also, I don't think margin and padding should be capitalized in the switch help text.

Updated.

The help text under each of the controls is redundant (Select the padding for this block).

Agreed. This was a hangover and is redundant. Removed.

The labels could be simplified. Try Mobile instead of Padding (mobile devices).

I think this removes a lot of noise from the UI. Updated.

I assume the double arrow icon on the right resets the setting but I'm not sure. Might be better to use a text button that reads "Reset".

Yep that's right. I've converted to a true Button with text and also disabled when there is no value currently selected.

I don't know what N and X do.

Addressed above.

Here's a screenshot for reference:

Screen Shot 2019-07-26 at 10 56 19

@bharath

This comment has been minimized.

Copy link

commented Jul 26, 2019

Hi, I have some thoughts to share on this.

I agree predefined options are a good start and specifying absolute values as stated can be added as an advanced option later.

But I think the option to specify those predefined options for each "side" individually should also be there from the start.

A lot of themes have full-width sections where only padding on the top and bottom is needed.

And also sometimes different values are used on the top and bottom.

@mtias

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Thanks for working on this @getdave. We already experimented with a similar UI for font sizes and switched away to a dropdown menu due to lack of clarity and internationalization issues. Can we try that here?

@mtias

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Also, I'd be inclined to separate this into two PRs because the device specific sizes need a bit more thinking across the entire UI.

@karmatosed

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Also, I'd be inclined to separate this into two PRs because the device specific sizes need a bit more thinking across the entire UI.

I would plus one this as it's getting huge. I also agree with the selection of sizes. Overall this feels a lot to take in visually and I think there should be a step back to consider what this would be like for someone coming to it. Are they going to understand all these settings? Are we using the right terms?

A prime example of how much this becomes is when you show all devices and this is what see on my 13 inch screen (I would have to scroll):

image

I was very confused by the following icon and it didn't seem to do anything.

Artboard2

Overall there is some key spacing to adjust and this demonstrates visually how this is all a lot:

Artboard

@@ -6,4 +6,181 @@
margin-top: 0;
margin-bottom: 0;
}

This comment has been minimized.

Copy link
@kjellr

kjellr Jul 26, 2019

Contributor

This is going to be really difficult for themes to override if they need to: there are a ton of different breakpoint/margin/padding combinations, and I worry that the theme would essentially need to rewrite all of them. This should work out of the box in many themes (it seems to be okay in Twenty Nineteen based on my testing), but will definitely break some too. For instance, allowing users to adjust the margins in the Gutenberg starter theme causes it to completely fall apart, since it relies on all blocks having auto left and right margins. For that reason, I wonder if this is this "Allow users to control margin and padding for individual blocks" is something that themes opt-in to? Similar to how they opt into wide alignments currently.

On a related note, most themes won't actually use the exact same breakpoints we use here. In #13363, we talked about potentially allowing themes to specify breakpoints for each of these devices. I'm not sure how/if that would best be implemented though. 馃

This comment has been minimized.

Copy link
@getdave

getdave Jul 26, 2019

Author Contributor

This is going to be really difficult for themes to override if they need to

I've got to be honest, I've kind of hacked this in, in the hope that someone more famliar with Themes can advise on how best to implement to keep specificity down and allow Themes to extend. I'd greatly value any commits you'd like to push in this direction.

On a related note, most themes won't actually use the exact same breakpoints we use here

Oh god, I didn't think of that. Of course, the Block Editor uses standard breakpoints, but Themes will choose their own.

I agree that we need a way to allow users to opt-in and to specify breakpoints. This seems like a wider project.

What route do we have to allow an initial MVP implementation of Block spacing to be shipped? Or do we need to get the breakpoint registration stuff done first?

Thanks!

This comment has been minimized.

Copy link
@kjellr

kjellr Jul 26, 2019

Contributor

I've got to be honest, I've kind of hacked this in, in the hope that someone more famliar with Themes can advise on how best to implement to keep specificity down and allow Themes to extend. I'd greatly value any commits you'd like to push in this direction.

I'll have to think about this a little bit. I wonder if @allancole, @laurelfulford, or @davidakennedy has any thoughts too. In general, I think we'll have much more leeway if we make this an opt-in for themes.

What route do we have to allow an initial MVP implementation of Block spacing to be shipped? Or do we need to get the breakpoint registration stuff done first?

I wonder if shipping the automatically-responsive spacing makes the most sense as an MVP. Then we can tackle the breakpoint-specific settings alongside theme-supplied breakpoints in a separate, wider issue.

This comment has been minimized.

Copy link
@getdave

getdave Jul 26, 2019

Author Contributor

I'll have to think about this a little bit

Thanks 馃憤

I wonder if shipping the automatically-responsive spacing makes the most sense as an MVP. Then we can tackle the breakpoint-specific settings alongside theme-supplied breakpoints in a separate, wider issue.

Agreed and I think this is what I will do. So MVP will ship with basic controls (no responsive). Then I'll work on branches for

  • a generic component (as seen here but not extracted)
  • the <DimensionControl /> component (as seen here)

Then when we've had a chance to think about the impact of responsive settings on Themes we can look to bring it all together.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Thanks for working on this @getdave.

@mtias Thanks for this. Much appreciated.

We already experimented with a similar UI for font sizes and switched away to a dropdown menu due to lack of clarity and internationalization issues. Can we try that here?

Yep, that makes sense. I'm starting to think this might be a good way to reduce the visual noise of the UI.

The main downside I can see is that it would require x2 clicks to toggle between different sizes. It is nice to be able to just click around and see the Block update in real time. The dropdown will mean a lot of clicks to experiment with different sizes.

On balance however, I suspect we'll end up going with the dropdown for the reasons you've stated.

Also, I'd be inclined to separate this into two PRs because the device specific sizes need a bit more thinking across the entire UI.

@mtias Can I confirm you're recommending

  • PR for a UI for Responsive controls (toggle to show/hide responsive)
  • PR for Dimension controls
  • PR for adding spacing controls to the Group Block

==========

Overall this feels a lot to take in visually...

@karmatosed I agree. As a non-designer, I'd definitely value and appreciate guidance from more visually talented folks.

...and I think there should be a step back to consider what this would be like for someone coming to it.

I agree and this is the moment to do that. I've demonstrated one implementation based on @kjellr's visuals. I'd love to receive alternative suggestions and options.

A prime example of how much this becomes is when you show all devices and this is what see on my 13 inch screen (I would have to scroll):

It's certainly something to consider. I agree there is a lot to take in. However, having everything open is something of an advanced use case. Indeed, I'm not sure it is that terrible to see a lot of controls, especially as they are clearly labelled and divided up.

I also think it's preferable for controls to visible by default rather than (unnecessarily) hiding the responsive UI controls behind toggles/switches as some other Block frameworks do. I'd rather the "noise" than make the UI overly complex or unintuitive.

We could reduce the noise by adopting the dropdown/select approach that @mtias suggests. Please see my comments on that above.

Other than that, do you have any alternatives we could try to reduce the visual noise? Much appreciated.

I was very confused by the following icon and it didn't seem to do anything.

I think you're looking at an out of date branch. Pull down the latest changes to see the updates.

You will see that - as per @drw158's comments - the reset UI is now clearly labelled and only active once a size is selected. I agree it was previously confusing 馃槃

Also, happy to fine-tune the spacing and alignment once the core UI has been agreed. Based on previous experience, I'm sure you'll understand that I'm disinclined to commit time to fine-tuning UI that hasn't got general approval from the community. I agree this is important to get right however 馃憤

Thanks for taking the time to feedback. It's much appreciated.

@karmatosed

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

However, having everything open is something of an advanced use case. Indeed, I'm not sure it is that terrible to see a lot of controls, especially as they are clearly labelled and divided up.

Whilst yes to interact would be advanced it was pretty easy and pretty much an expected interaction pattern got me to them all being open. In this case I am not sure it is clear how things are labelled and divided up, so that is absolutely something to work on.

I think you're looking at an out of date branch. Pull down the latest changes to see the updates.

I pulled a few hours ago but can do again.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

But I think the option to specify those predefined options for each "side" individually should also be there from the start.

@bharath Thanks for this. I understand why you bring it up. It's difficult to balance ideal functioning vs essential functionality. I feel we should be able to introduce this fairly easily in future PRs once the initial implementation is complete. Again, it's about not over-complicating the initial implementation (I say this as the person who has a PR with 19 files changed! 馃槃 ).

@mtias What say you to this?

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Whilst yes to interact would be advanced it was pretty easy and pretty much an expected interaction pattern got me to them all being open. In this case I am not sure it is clear how things are labelled and divided up, so that is absolutely something to work on.

@mtias @karmatosed Whilst still in this branch, I've been experimenting with using select controls as per Matias' suggestion. Here's a screenshot of this WIP. It does remove a lot of visual noise.

Select based UI

Screen Shot 2019-07-26 at 15 33 40

Button Group based UI

Screen Shot 2019-07-26 at 10 56 19

Do you feel this is an improvement?

@getdave getdave referenced this pull request Jul 29, 2019
0 of 5 tasks complete
@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Following multiple requests, this is being broken up into separate PRs.

See description.

getdave added a commit that referenced this pull request Jul 29, 2019
Adds initial component
Note this is copied wholescale from original PR #16730
@getdave getdave referenced this pull request Jul 29, 2019
5 of 5 tasks complete

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

@paaljoachim

This comment has been minimized.

Copy link

commented Sep 4, 2019

Hey Dave @getdave
it would be great with an update to where this is at and how it can be moved forward.
Thanks!

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Hey. I've been away. Planning to try and give this some attention soon.

getdave added a commit that referenced this pull request Sep 14, 2019
Adds initial component
Note this is copied wholescale from original PR #16730
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.