Button Block: Added configurable text color. #3034
Conversation
jorgefilipecosta
added
Needs Design Feedback
Codecov Report
@@ Coverage Diff @@
## master #3034 +/- ##
=======================================
Coverage 32.76% 32.76%
=======================================
Files 203 203
Lines 5951 5951
Branches 1052 1052
=======================================
Hits 1950 1950
Misses 3375 3375
Partials 626 626
Continue to review full report at Codecov.
|
blocks/library/button/index.js
Outdated
| @@ -76,7 +79,7 @@ registerBlockType( 'core/button', { | |||
| <BlockAlignmentToolbar value={ align } onChange={ updateAlignment } /> | |||
| </BlockControls> | |||
| ), | |||
| <span key="button" className={ className } title={ title } style={ { backgroundColor: color } } > | |||
| <span key="button" className={ className } title={ title } style={ { backgroundColor: backgroundColor } } > | |||
There was a problem hiding this comment.
Minor: could be written { backgroundColor }
blocks/library/button/index.js
Outdated
| textColor: { | ||
| type: 'string', | ||
| }, | ||
| backgroundColor: { |
There was a problem hiding this comment.
Changing the attribute's name make the "old" buttons block for existing posts invalid, Maybe we should just keep the name as is
blocks/library/button/index.js
Outdated
| <ColorPalette | ||
| value={ backgroundColor } | ||
| onChange={ ( colorValue ) => setAttributes( { backgroundColor: colorValue } ) } | ||
| withTransparentOption |
There was a problem hiding this comment.
I think this prop is not used anymore, we should just drop it
blocks/library/button/index.js
Outdated
| <div className={ `align${ align }` } style={ { backgroundColor: color } }> | ||
| <a href={ url } title={ title }> | ||
| <div className={ `align${ align }` } style={ { backgroundColor: backgroundColor } }> | ||
| <a href={ url } title={ title } style={ { color: textColor } }> |
There was a problem hiding this comment.
I wonder what HTML would this produce if textColor is undefined. My thinking is that it should not add any style attribute to the dom node to keep the old blocks "valid"
There was a problem hiding this comment.
Tested and works as expected
There was a problem hiding this comment.
I tested the HTML produced and if textColor is null no style is added.
jorgefilipecosta
force-pushed
the
add/configurable-text-color-button
branch
from
9ab3088
to
225c28c
Compare
jorgefilipecosta
force-pushed
the
add/configurable-text-color-button
branch
from
225c28c
to
f3aca2a
Compare
|
Thank you for the feedback @youknowriad 👍 I think I addressed the concerns raised. |
|
@jorgefilipecosta right now I am seeing this:
This is after the button contrast check merge, I'm wondering if it's impacted by that? |
|
Hi @karmatosed, the contrast verification was only applied to button it did not affect the paragraph. In the screens, I see that a paragraph is selected with the same color for background and text and no contrast verification is appearing (because it was not yet added to paragraph). As long as the colors were the chosen ones, I'm not being able to see other problem, could you please describe the wrong behavior? So I can try to find the cause. |
|
Update: after chatting to @jorgefilipecosta it seems the paragraph hasn't been added. I was mistaken on seeing the issue closed. Thanks for confirmation. |
Description
This PR adds the option to set the text color of a button as specified in issue #2766.
Testing instructions
Screenshots:
Some notes:
The background and text color configurators are very similar to the ones in the paragraph, and other components will probably have it too. I think maybe creating a new component to choose the colors for background and text may be a good idea. This component than would be able to have a contrast verification feature as being discussed in #2381. If you prefer this approach let me know, and I will create a new component and use it in paragraph and button.