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

Combine color palettes into a single settings panel for Button and Paragraph blocks #7924

Merged
merged 7 commits into from
Jul 27, 2018

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jul 12, 2018

Description

Closes #6255

Combines ColorPalettes into a single panel for Button and Paragraph blocks.

Changes:

  • Carried out some refactoring of Button block to make FallbackStyles a HOC of the editor component instead of the ContrastChecker
  • Extracted ColorIndicator into its own component from PanelColor component
  • Created new PanelColorSettings component that displays both Background and Text color palettes and a ContrastChecker
  • Added styling so that ColorAreas are only displayed on panel title when the panel is collapsed

How has this been tested?

  • Manual testing
  • Added snapshot tests

Screenshots

combined-color-palette

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.

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks labels Jul 12, 2018
@talldan talldan self-assigned this Jul 12, 2018
@gziolo gziolo requested a review from jorgefilipecosta July 12, 2018 12:00
@talldan talldan changed the title Update/merge color settings panes Combine color palettes into a single settings panel for Button and Paragraph blocks Jul 13, 2018
*/
import './style.scss';

const ColorArea = ( { ariaLabel, colorValue, className } ) => (
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 16, 2018

Choose a reason for hiding this comment

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

Maybe we can call this component something like ColorIndicatorSquare to make its name more specific. We already have many color components; it may not be easy to guess what color area is.

const ColorArea = ( { ariaLabel, colorValue, className } ) => (
<span
className={ classnames( 'component-color-area', className ) }
aria-label={ ariaLabel }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the component can pass all the additional props to span and aria-label is just an additional prop.

e.g:

const ColorArea = ( { colorValue, className, ...props } ) => (
...
<span 
	{ ...props }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suprisingly, React doesn't convert ariaLabel to aria-label for those built-in elements, so this won't work.

Copy link
Member

Choose a reason for hiding this comment

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

I think it works if the users of the component pass:

<ColorArea
	aria-label=""
...
/>

Users will need to set aria-label and not ariaLabel.

onChange={ setTextColor }
<PanelColorSettings
title={ __( 'Color Settings' ) }
textColorProps={ {
Copy link
Member

Choose a reason for hiding this comment

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

The fact that we have special props for background and text makes this component specific to this colors and impossible to use for a border color for example.

What if we change the API a little bit to something like:

<PanelColorSettings
	title={ __( 'Color Settings' ) }
	colorZones={ [ {
		label: title={ __( 'Background Color' ) }
		value: backgroundColor.value,
		onChange: setBackgroundColor,
	},	{
		label: title={ __( 'Text Color' ) }
		value: textColor.value,
		onChange: setTextColor,
	},
	{
		label: title={ __( 'The Other Color My block Needs' ) }
		value: otherColor.value,
		onChange: setOtherColor,
	}

] }
>
	<ContrastCheckerWithFallbackStyles
			node={ this.nodeRef }
			textColor={ textColor.value }
			backgroundColor={ backgroundColor.value 
			isLargeText={ true }
	/>
</PanelColorSettings>

The component would render, all the childs passed so the contrast checking would be something outside of the scope of the component.

className={ baseClassName }
title={ getTitle( panelTitleClassName, title, [ backgroundColorAreaProps, textColorAreaProps ] ) }
>
<BaseControl
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 16, 2018

Choose a reason for hiding this comment

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

Given that we have a component just for the square indicator we should probably have a component for the color zone with its title.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Nice work here @talldan, thank you for this changes.

I did some tests, and from the functional point of view, this is working great.

The only detail remaining is that before on the paragraph block the color was collapsed by default, and now, it is opened by default. I think we can fix this by adding an initialOpen prop to the color settings component.

I left some ideas about other approaches for the API we offer to block creators.

@talldan talldan force-pushed the update/merge-color-settings-panes branch 2 times, most recently from 4709eb9 to 76e55a1 Compare July 17, 2018 04:31
@talldan
Copy link
Contributor Author

talldan commented Jul 17, 2018

@jorgefilipecosta thanks for the review! I did quite a bit of refactoring based on your comments. I've extracted almost everything from the main PanelColorSettings component and that now just maps over the colorSettings and renders other components.

I'm fairly happy with how it looks now, apart from maybe naming and also the widespread use of withColorContext HOC (which I think should be moved to the editor/components/colors folder).

Would we need to deprecate the existing version to move it? Maybe a separate PR?

I think I could also add some more test coverage for my changes, I can do that if you're happy with the changes.

/> }
<PanelColorSettings
title={ __( 'Color Settings' ) }
colorSettings={ [
Copy link
Member

Choose a reason for hiding this comment

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

To note, The only reason we are passing the color settings as an array of objects instead of using directly a React component is because if we pass as settings object we can use the same values for the color indicators and avoid passing repeated information. Another approach would be access child components of PanelColorSettings from PanelColorSettings which seems an anti-pattern or using slot-fill which seems complex for this case. Maybe we can add some comments to PanelColorSettings explaining this.
I like this version.

onChange: setBackgroundColor,
label: __( 'Background Color' ),
// translators: %s: The name of the color e.g: "vivid red" or color hex code if name is not available e.g: "#f00".
colorIndicatorAriaLabel: __( '(current background color: %s)' ),
Copy link
Member

Choose a reason for hiding this comment

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

I would like to avoid the need for colorIndicatorAriaLabel, or at least make it optional. Can we derive the aria-label from the label, and maybe even remove current. e.g: use (background color: %s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does that work with i18n? I thought the strings could only be statically analysed when using __.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use something like: '(current %s: %s)' or '(%s: %s). Where we specify in the translator comments that the first string refers to the color label in lower case e.g: 'background color' and the second label to the color name or value.

/>
);

const TextWithColorIndicators = ( { className, colorSettings, children } ) => (
Copy link
Member

Choose a reason for hiding this comment

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

Can we add this component inside the PanelColorSettings and not export it? It is very simple and each component we expose will require maintenance in the future in case we do more changes and we need to deprecate it.

import withColorContext from '../color-palette/with-color-context';
import { getColorName } from '../colors';

export default withColorContext( ( { colors, colorValue, ariaLabel } ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, in this case, we don't need the editor/components/color-indicator/index.js, the only logic operation it is doing before using components/color-indicator is the computation of the ariaLabel, components using this component can easily compute the aria-label and we avoid another component to maintain.

@aduth aduth added this to the 3.4 milestone Jul 18, 2018
@talldan talldan force-pushed the update/merge-color-settings-panes branch from ad1a113 to 32aa85d Compare July 20, 2018 09:00
@talldan
Copy link
Contributor Author

talldan commented Jul 20, 2018

@jorgefilipecosta I think I've addressed all those comments. Thanks for the re-review.

@talldan talldan force-pushed the update/merge-color-settings-panes branch from 4c92b62 to df6f890 Compare July 23, 2018 03:14
@talldan
Copy link
Contributor Author

talldan commented Jul 23, 2018

Have resolved the merge conflict (which was from your change to the contrast checker @jorgefilipecosta), and fixed a few issues with tests that I managed to somehow be oblivious to on Friday.

// translators: first %s: The type of color (e.g. background color), second %s: the color name or value (e.g. red or #ff0000)
const colorIndicatorAriaLabel = __( '(current %s: %s)' );

const ColorPaletteControl = withColorContext( ( { label, value, onChange, colors } ) => {
Copy link
Contributor Author

@talldan talldan Jul 23, 2018

Choose a reason for hiding this comment

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

I guess this component could still be moved into PanelColorSettings, though it does feel like it might be useful as a separate component.

@talldan talldan force-pushed the update/merge-color-settings-panes branch 2 times, most recently from 36e5b44 to 2874f0f Compare July 24, 2018 02:31
// translators: first %s: The type of color (e.g. background color), second %s: the color name or value (e.g. red or #ff0000)
const colorIndicatorAriaLabel = __( '(current %s: %s)' );

const ColorPaletteControl = withColorContext( ( { label, value, onChange, colors } ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Normally we apply the HOC's when doing the export
e.g: export default withColorContext ( ColorPaletteControl );

import { getColorName } from '../colors';

// translators: first %s: The type of color (e.g. background color), second %s: the color name or value (e.g. red or #ff0000)
const colorIndicatorAriaLabel = __( '(current %s: %s)' );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Given that this is a constant outside any component/function, maybe we should call it: COLOR_INDICATOR_ARIA_LABEL.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

The code seems to be working well 👍
Thank you @talldan for iterating on this. I think we should merge this soon, so we have more eyes on this changes before the next release.
We are not using now some components like PanelColor; I think these components should be deprecated after this merge. Plugin developers ideally should use the same components we use in core so we have a uniform UI, now the component to use should be PanelColorSettings.

talldan added 2 commits July 27, 2018 15:00
…or text

Also bring title color rendering that was previously in another PanelColor
component into this one.

Next ColorPallettes in BaseControl components

Extract repeated code for building color labels into separate function

Handle change events for both text and bg colors

Uncouple the FallbackStyles HOC from the ContrastChecker in the Button block

Reorganise props to PanelColor, group text and background props into objects

Move ContrastChecker into PanelColor component

Remove incorrect class name

Move title rendering into a separate func and use Fragement over array

Reintroduce the old PanelColor component and rename this new one PanelTextColor

Extract PanelBody for text color panel into its own component

Hide/show text color preview dependent on the open/close state of the panel

Extract ColorArea into its own component so that it can be used in more than just panels

Revert "Extract PanelBody for text color panel into its own component"

This reverts commit fe7a803.

Refactor PanelTextColor component to reuse title for both PanelBody and BaseControl

 Relocate styles for hiding/showing ColorAreas to PanelTextColor

Modify ColorPalette component to accept an additional class name

Styling tweaks for text color pallete

Fix incorrect class name on ColorArea

Refactoring to remove need to add class to ColorArea

Minor tidy up to avoid use of not needed consts

Rename from PanelTextColor to PanelColorSettings

Update PanelColor tests to reflect use of ColorArea component

Add snapshot tests for new components

Minor formatting change

Fixes for FallbackStyles HOC usage on button.

Migrate to new compose package for PanelColorSettings and button block

Rename ColorArea to ColorIndicator

Make ContrastChecker render as a child of the PanelColorSettings component

Refactor PanelColorSettings to accept an array of settings

Make TextWithColorIndicators a component

Fix classname typo - I can't spell 'palette'.

Remove unecessary span in favour of Fragment

Refactor, avoid passing colors through several levels

Extract ColorPaletteControl, TextWithColorIndicators and a ColorIndicator that has color context from PanelColorSettings

Set initialOpen for color palettes to false for Paragraph block
@talldan talldan force-pushed the update/merge-color-settings-panes branch from 2874f0f to 69971d6 Compare July 27, 2018 07:39
@talldan talldan merged commit 288fe78 into master Jul 27, 2018
@talldan talldan deleted the update/merge-color-settings-panes branch July 27, 2018 08:58
@aduth
Copy link
Member

aduth commented Apr 22, 2019

Is ColorPaletteControl meant to be made publicly available? Could you advise on #13018?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge background and text color panels
3 participants