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

Introduce colors slugs to ensure i18n #7319

Merged
merged 27 commits into from Jul 3, 2018

Conversation

@Luehrsen
Contributor

Luehrsen commented Jun 15, 2018

Description

This PR builds ontop of the work being done in #7145 to ensure legibility of color strings across language barriers.

How has this been tested?

This code has been tested locally.

Types of changes

This PR extends the editor-color-palette so that color items now have the additional slug key.

{
  name: __( 'pale pink' ),
  slug: 'pale pink',
  color: '#f78da7',
}

Checklist:

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

@tofumatt tofumatt requested a review from jorgefilipecosta Jun 17, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 18, 2018

Should we use slug as the new way for searching and fill it in with the value provided for name in case it doesn't exist for backward compatibility? It might simplify the code a bit.

@Luehrsen

This comment has been minimized.

Contributor

Luehrsen commented Jun 18, 2018

You mean like switching this around or do you have anything more substantial in mind?

const colorObj = ( find( colors, { name: namedColor } ) || find( colors, { slug: namedColor } ) );

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 18, 2018

You mean like switching this around or do you have anything more substantial in mind?

const colorObj = ( find( colors, { name: namedColor } ) || find( colors, { slug: namedColor } ) );

I meant to use:

const colorObj = find( colors, { slug: namedColor } );

To do that the only change that is required, is to make sure that slug is always provided from PHP. In effect, if there is no slug, you can copy its value from name.

@Luehrsen

This comment has been minimized.

Contributor

Luehrsen commented Jun 19, 2018

@gziolo I've implemented that. You're right, that is much cleaner. I've also updated the readme to reflect the changes.

@@ -1139,7 +1139,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
}
if ( ! empty( $color_palette ) ) {
$editor_settings['colors'] = $color_palette;
$editor_settings['colors'] = apply_filters( 'editor_color_palette', $color_palette );

This comment has been minimized.

@gziolo

gziolo Jun 19, 2018

Member

This is cool, thanks for applying changes to this PR. In my opinion, it reads better on JS side and removes some unnecessary complexity.

Just wanted to double check, if we really need another filter in here? Maybe it would be enough to call this function directly and defer filter when there are requests from plugin developers. Otherwise, we will have to document it and what's worse maintain forever. It seems like get_theme_support( 'editor-color-palette' ) should offer enough control. This change is needed mostly for backward compatibility.

This comment has been minimized.

@Luehrsen

Luehrsen Jun 19, 2018

Contributor

IMHO there really can't be enough filters. This gives plugins and child-themes the chance to extend/modify an existing set of colors. Together with a class like phpColors I can imagine the possibilities to be amazing.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 19, 2018

Member

I'm not sure if we should have this filter right now or not, it is another artifact to maintain, and even if we don't add it now, we can add it later. Also if we add this filter here, we may need for consistency reasons to add filters to other theme settings.
Another discussion to have on this kind of filters is if they should be PHP or client-side JavaScript filters. The client side filters offer the advantage of allowing plugins/themes to have UI that dynamically changes the values during Gutenberg execution. It would be possible to vary color palette depending on post size, post format or tags for example.

If we decide to use the filter I feel it should be called even if the color palette was not set (with an empty array) so filters can add colors. After filter execution, we verify if the colors are not empty and in that case, we set the editor colors if they are not empty we don't set them (so no bytes travel and Gutenberg uses the default colors).

This comment has been minimized.

@Luehrsen

Luehrsen Jun 19, 2018

Contributor

Should we remove that filter and move that discussion to a later date?

This comment has been minimized.

@gziolo

gziolo Jun 19, 2018

Member

Yes, please. Let's open a separate issue for that.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 19, 2018

I had only one comment, otherwise, everything looks great. It's a very nice improvement and addressed the issue described by @felixarntz in #7145. @jorgefilipecosta, can you do a sanity check before we proceed?

@gziolo gziolo requested a review from felixarntz Jun 19, 2018

@gziolo gziolo added this to the 3.1 milestone Jun 19, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 19, 2018

Let's try to get this PR included in 3.1 release so we could consider this API final.

@mtias

This comment has been minimized.

Contributor

mtias commented Jun 19, 2018

Looping @mor10 quickly as we chatted about this API for themes.

@@ -15,46 +20,57 @@ export const EDITOR_SETTINGS_DEFAULTS = {
alignWide: false,
colors: [
{
name: 'pale pink',
name: __( 'pale pink' ),
slug: 'pale pink',

This comment has been minimized.

@mtias

mtias Jun 19, 2018

Contributor

Are these supposed to have dashes?

This comment has been minimized.

@Luehrsen

Luehrsen Jun 19, 2018

Contributor

they probably should, but they get sanitized a bit further down the road.

So, yes for understandability, no for actual need.

@Luehrsen

This comment has been minimized.

Contributor

Luehrsen commented Jun 19, 2018

What a coincidence, I also talked about that with @mor10 on wednesday morning I believe. That discussion made me do this patch.

@Luehrsen Luehrsen closed this Jun 19, 2018

@Luehrsen Luehrsen reopened this Jun 19, 2018

@@ -15,7 +15,7 @@ import { find, kebabCase } from 'lodash';
*/
export const getColorValue = ( colors, namedColor, customColor ) => {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 19, 2018

Member

Should we rename the parameter from namedColor to colorSlug or slug? (the same for other similar parameters)

This comment has been minimized.

@Luehrsen

Luehrsen Jun 19, 2018

Contributor

I would say no, as the difference is namedColor to customColor, and the 'named' part is talking about the type of color, not 'name' or 'slug'.

This comment has been minimized.

@felixarntz

felixarntz Jun 26, 2018

Member

I think it makes sense to rename it to slug or colorSlug, as it is precisely that (see for example how the value is used as slug in the function.

This comment has been minimized.

@Luehrsen

Luehrsen Jun 28, 2018

Contributor

Again, I would disagree, as namedColor is not a reference to name or slug, but a reference that we are talking about a color defined by the theme and not custom. A better disambiguation would probably be definedColor and customColor.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 19, 2018

It looks like we need to remove the filter discussed in here #7319 (comment) and we can move forward. It'd be great to get feedback from @mor10 before we merge, but I wouldn't consider it a blocker. We can always iterate :)

@@ -15,7 +15,7 @@ import { find, kebabCase } from 'lodash';
*/
export const getColorValue = ( colors, namedColor, customColor ) => {
if ( namedColor ) {
const colorObj = find( colors, { slug: namedColor } );
const colorObj = ( find( colors, { slug: namedColor } ) || find( colors, { name: namedColor } ) );
return colorObj && colorObj.color;

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

@jorgefilipecosta @Luehrsen, how do you feel about adding a deprecation note in here so we could inform about the breaking changes introduced? I wasn't aware of that fact until now. It seems like a good idea to keep JS console noisy for 2-3 releases to help them migrate to the latest API structure.

@aduth aduth modified the milestones: 3.1, 3.2 Jun 21, 2018

let colorObj = find( colors, { slug: namedColor } );
if ( typeof colorObj === 'undefined' && typeof ( colorObj = find( colors, { name: namedColor } ) ) !== 'undefined' ) {
deprecated( 'Using color objects without slugs', {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 25, 2018

Member

Thank you for all the updates, and thank you for coming up a deprecation method that is transparent for the users @Luehrsen, this is looking good!
Maybe can use 'Referencing color by name in attributes when a slug exists.' as the message because we are not deprecating color objects without slugs. Color can still be set without slugs and an automatic slug is computed which is fine for single language sites.

This comment has been minimized.

@felixarntz

felixarntz Jun 26, 2018

Member

+1 for this implementation.

@jorgefilipecosta I agree that in this specific case here your suggestion is more fitting as a message, however I thought we do deprecate color objects without slugs. Of course they remain working, but providing colors without a slug is wrong. Sure it works for single language sites, but we should have multilingual sites or sites where they switch the language at some point in mind and both educate and enforce solid patterns to support these.

@felixarntz

I tested this PR, also looking particularly at the a11y color contrast integration. Everything continues to work as expected, the only thing that needs to be added as mentioned before, is a migration routine to keep the old colors still selected.

In addition, I think we should be vocal about deprecating colors specified without a slug.

Other than that, great work so far!

@@ -15,7 +15,7 @@ import { find, kebabCase } from 'lodash';
*/
export const getColorValue = ( colors, namedColor, customColor ) => {

This comment has been minimized.

@felixarntz

felixarntz Jun 26, 2018

Member

I think it makes sense to rename it to slug or colorSlug, as it is precisely that (see for example how the value is used as slug in the function.

let colorObj = find( colors, { slug: namedColor } );
if ( typeof colorObj === 'undefined' && typeof ( colorObj = find( colors, { name: namedColor } ) ) !== 'undefined' ) {
deprecated( 'Using color objects without slugs', {

This comment has been minimized.

@felixarntz

felixarntz Jun 26, 2018

Member

+1 for this implementation.

@jorgefilipecosta I agree that in this specific case here your suggestion is more fitting as a message, however I thought we do deprecate color objects without slugs. Of course they remain working, but providing colors without a slug is wrong. Sure it works for single language sites, but we should have multilingual sites or sites where they switch the language at some point in mind and both educate and enforce solid patterns to support these.

foreach ( $color_palette as $color ) {
if ( ! isset( $color['slug'] ) ) {
$color['slug'] = esc_js( $color['name'] );

This comment has been minimized.

@felixarntz

felixarntz Jun 26, 2018

Member

I think we should preferably throw a _doing_it_wrong() notice here, in addition to auto-generating the slug. As mentioned before, one should always specify colors including a slug.

}
$new_color_palette[] = $color;
}
if( $is_doing_it_wrong ) {

This comment has been minimized.

@gziolo

gziolo Jun 28, 2018

Member

phpcs will complain :)

This comment has been minimized.

@Luehrsen

Luehrsen Jun 28, 2018

Contributor

🤨

if ( $is_doing_it_wrong ) {
_doing_it_wrong(
'add_theme_support/editor-color-palette',

This comment has been minimized.

@felixarntz

felixarntz Jun 28, 2018

Member

This needs to be the function name only, i.e. add_theme_support(). I agree though that it makes sense to highlight the theme feature, so maybe in the description you could say "Each color specified for the editor color palette...".

if ( $is_doing_it_wrong ) {
_doing_it_wrong(
'add_theme_support',

This comment has been minimized.

@felixarntz

felixarntz Jun 28, 2018

Member

Sorry, I should have been more clear. You need to include parentheses, as in 'add_theme_support()'. I think the description may be more precise saying 'Each color in the "editor-color-palette" should have a slug defined.' - the latter is my opinion though, let me know if you disagree for some reason.

if ( $is_doing_it_wrong ) {
_doing_it_wrong(
'add_theme_support',
__( 'The "editor-color-palette" should have color slugs defined.', 'gutenberg' ),

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 28, 2018

Member

In comment #7319 (comment) I proposed an alternative to requiring themes to set slugs. I would love to hear more thoughts on it.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 28, 2018

@jorgefilipecosta - feel free to merge if you are happy with the current state. I think we are in good shape 👍 @Luehrsen, we came a long way, props 💯

@gziolo gziolo added this to To do in Extensibility via automation Jun 28, 2018

/**
* Ensure the editor module is loaded before third party plugins.
*
* Remove this in Gutenberg 3.1

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 28, 2018

Member

Should we remove this lines now?

This comment has been minimized.

@Luehrsen

Luehrsen Jul 2, 2018

Contributor

Can do, but isn't that technically a task for a separate "Remove Deprecations" PR?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jul 2, 2018

Member

According to the github diff, I think this lines:

 * Ensure the editor module is loaded before third party plugins.
 *
 * Remove this in Gutenberg 3.1
 */
function polyfill_blocks_module_in_scripts() {
	if ( ! is_gutenberg_page() ) {
		return;
	}
 
	wp_enqueue_script( 'wp-editor' );
}
 
add_action( 'enqueue_block_editor_assets', 'polyfill_blocks_module_in_scripts', 9 );
add_action( 'enqueue_block_assets', 'polyfill_blocks_module_in_scripts', 9 );
...

Are being added in this PR, and they don't look something related to the colors, it was probably a problem during rebase/merge.

This comment has been minimized.

@gziolo

gziolo Jul 2, 2018

Member

Yes, this shouldn't be included in this PR. It was already removed.

This comment has been minimized.

@gziolo

gziolo Jul 3, 2018

Member

I removed it myself.

This comment has been minimized.

@Luehrsen

Luehrsen Jul 3, 2018

Contributor

Was about to do it. Thank you!

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jul 3, 2018

Member

Thank you for the changes @gziolo, I was not aware we could add commits in other person forks. That is nice to know 👍

This comment has been minimized.

@gziolo

gziolo Jul 3, 2018

Member

Yes, it's very useful when there is minor update blocking PR. 😃

@gziolo gziolo merged commit 6c0dbfd into WordPress:master Jul 3, 2018

2 checks passed

codecov/project 46.77% (-0.07%) compared to 5389ada
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Extensibility automation moved this from To do to Done Jul 3, 2018

@Luehrsen Luehrsen deleted the luehrsenheinrich:fix/introduce-color-slugs branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment