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 background color support to Columns block #17813

Open
wants to merge 15 commits into
base: master
from

Conversation

@richtabor
Copy link
Contributor

richtabor commented Oct 7, 2019

Description

Add background color support to the core Columns block, in support of #16660. This uses the same method employed by the core Group block.

How has this been tested?

Tested in supported browsers and ensured color classes and custom colors are applied to the columns block within the editor and the front-end.

Screenshots

colors

Types of changes

New feature: A new Color Settings panel for the Columns block, allowing folks to add a background color to their Column blocks.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@CreativeDive

This comment has been minimized.

Copy link
Contributor

CreativeDive commented Oct 7, 2019

@richtabor Nice work. But how we can change the text color in relation to the background color?

@richtabor

This comment has been minimized.

Copy link
Contributor Author

richtabor commented Oct 7, 2019

@richtabor Nice work. But how we can change the text color in relation to the background color?

It's tricky when we're styling nested blocks. I lean towards letting child blocks handle their own color styling, instead of having a priority style system. For example, you'd apply a background to the whole Columns block, but select your headings and color - then select your paragraphs and color.

@mapk
mapk approved these changes Dec 4, 2019
Copy link
Contributor

mapk left a comment

Great work! 😍 I tested this and it worked just right. :shipit: if the technical feedback is all good in the hood.

On a side note, I really want to change background colors on individual columns within the Columns block.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 4, 2019

I think this could be refactored to use the new useColors hook.

Also, I agree that background color options for individual Column blocks would be a cool addition for another PR.

@richtabor

This comment has been minimized.

Copy link
Contributor Author

richtabor commented Dec 17, 2019

@ZebulanStanphill I've updated the block to use useColors now. Mind taking a look to double-check? Thanks!

Copy link
Contributor

ZebulanStanphill left a comment

One minor comment, but other than that, everything seems to be working fine.

{
contrastCheckers: [ { backgroundColor: false } ],
}
Comment on lines 73 to 75

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Dec 18, 2019

Contributor

I don't understand what the purpose of including this is?

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Dec 18, 2019

I’d vote to allow text color as well. Without it it’s a little confusing how it all works - I rarely trust magic. This same discussion is happening for the Group block: #19181

@richtabor

This comment has been minimized.

Copy link
Contributor Author

richtabor commented Dec 18, 2019

@shaunandrews I wrote a comment on #19181 to highlight a concern with having a top-level TextColor control on a parent block (Columns, Group):

I'd appreciate your thoughts on it. :)

@richtabor

This comment has been minimized.

Copy link
Contributor Author

richtabor commented Dec 18, 2019

I’d vote to allow text color as well. Without it it’s a little confusing how it all works - I rarely trust magic. This same discussion is happening for the Group block: #19181

Also, what magic are you referring to? The auto contrast applied by the TwentyTwenty theme?

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Dec 18, 2019

Also, what magic are you referring to? The auto contrast applied by the TwentyTwenty theme?

Yes, exactly. Anytime the editor tries to do something for me, its attempting to be clever. Software is rarely great at being clever, and magic doesn't exist. I see this auto-text-color thing as an attempt at clever magic. For many use-cases, it might be a very clever solution that works — but for many other user-cases, it will just be confusing magic if it can't be overridden.

@richtabor

This comment has been minimized.

Copy link
Contributor Author

richtabor commented Dec 18, 2019

Yes, exactly. Anytime the editor tries to do something for me, its attempting to be clever.

Well that's per the theme, not from the color palette control within Gutenberg. Most themes don't do that actually.

@BinaryMoon

This comment has been minimized.

Copy link

BinaryMoon commented Dec 18, 2019

Currently you can change the background colour on columns, but this doesn't work properly when the column alignment is changed. Center vertical alignment and only the center section is coloured, not the whole column.

Can this be fixed here, or should it be a separate bug report?

@richtabor

This comment has been minimized.

Copy link
Contributor Author

richtabor commented Dec 20, 2019

Currently you can change the background colour on columns, but this doesn't work properly when the column alignment is changed. Center vertical alignment and only the center section is coloured, not the whole column.

Could you take a screen recording of this/provide browser details? I'm not getting the same behavior currently:

ScreenFlow

@BinaryMoon

This comment has been minimized.

Copy link

BinaryMoon commented Dec 20, 2019

@richtabor Hey Rich - I'm talking about individual columns, rather than the entire block. As I said, it may be (is likely) unrelated.

I made a little demo here: https://demo.prothemedesign.com/wordpress/gutenberg/2019/12/20/column-background-bug/

I expect the column background to stretch full height. Not to just wrap the content.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 21, 2019

@BinaryMoon Yeah, I think that's a separate matter from this PR. This PR only concerns background color for the entire Columns block.

(Personally, I think it's debatable whether or not individual columns should be full-height.)

@richtabor richtabor requested review from ZebulanStanphill and mapk Dec 22, 2019
@BinaryMoon

This comment has been minimized.

Copy link

BinaryMoon commented Dec 22, 2019

@ZebulanStanphill Thanks - I've added a new issue: #19295

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Dec 24, 2019

Seems like the questions have all been answered and clarified here. @richtabor if you're around, how about we get this merged?

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 24, 2019

I noticed an issue. If the Column blocks have nested Paragraph blocks and you use the custom color picker and drag the dot all the way to the top and try dragging beyond that, this happens:
image

This does not happen with the Group block.

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.