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 color options to heading block. #15625

Merged
merged 1 commit into from May 21, 2019

Conversation

jorgefilipecosta
Copy link
Member

Description

Closes: #6012

This PR adds the color options available in the Paragraph and button block to the heading block.

How has this been tested?

I verified the color options in the heading block work as expected.

@youknowriad youknowriad added [Block] Heading Affects the Headings Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement. labels May 14, 2019
@mapk
Copy link
Contributor

mapk commented May 15, 2019

I tested this and it worked great! :shipit:

heading-color

Let's merge this, but keep in mind that we may want to reevaluate the background color options for all these text blocks and just use the Group block to provide that.

@mapk mapk removed the Needs Design Feedback Needs general design feedback. label May 15, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the add/color-options-to-heading-block branch from e81c6b9 to 6e916e3 Compare May 15, 2019 18:41
@youknowriad
Copy link
Contributor

But keep in mind that we may want to reevaluate the background color options for all these text blocks

Do we have a decision here, because I'd rather not introduce something that is going to be removed (it's always hard to remove things when they become APIs)

@mapk
Copy link
Contributor

mapk commented May 17, 2019

Do we have a decision here?

Yes, we're going to keep this in. A decision is made to keep background color on most (if not all) text blocks. #8171 (comment)

Let's merge it.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

We could use an e2e test.

I wonder how much of this code could be reused across blocks.

@jorgefilipecosta jorgefilipecosta merged commit 22ab406 into master May 21, 2019
@jorgefilipecosta jorgefilipecosta deleted the add/color-options-to-heading-block branch May 21, 2019 09:53
@jorgefilipecosta
Copy link
Member Author

Thank you for the reviews @youknowriad, @mapk 👍

We could use an e2e test.

I will follow up with an end 2 end test for this functionality.

I wonder how much of this code could be reused across blocks.

We already have a big amount of code reusability, by using shared components for the UI, withColors for the logic, and shared functions. But I agree it is something we could try to improve further. The main decision we would need to do would be the way this reusability would work, the simplest option would be a supports flag for color, but in previous discussions, there was no clear agreement if we should continue investing in support options.

@BinaryMoon
Copy link

We really need this for all block controls (lists, paragraphs etc). Anything that can be used on a group block with a coloured background could cause readability issues and so the text colour will need to be changeable.

@mapk
Copy link
Contributor

mapk commented May 28, 2019

That's the next step here, @BinaryMoon. We want to add text color options to all text blocks.

@justintadlock
Copy link
Contributor

The .has-text-color class is missing. It only shows the .has-xxx-color class.

@paaljoachim
Copy link
Contributor

paaljoachim commented Jul 31, 2019

Heading background color was removed:
#15814

With the thought that "The idea being that backgrounds should be higher-level (group block)."

@youknowriad youknowriad mentioned this pull request Aug 19, 2019
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add text color option to Heading block
6 participants