Color Schemes: Add Color Scheme Picker #17200

frontdevde · 2017-08-15T01:53:51Z

This PR is part of a series of PRs aiming to provide support for Color Schemes in Calypso.
This PR focuses on adding the color scheme picker based on the components added in PR #17157.

Notable additions in this PR:

The <ColorSchemePicker> component allows for the selection of a color scheme.
When using the component as is, any selection will immediately be saved to the colorSchemes property in the calypso_settings. When the props temporarySelection and onSelection are provided, any selection will only be set locally and the provided onSelection handler called (e.g. to save the selection via a save button in a form).
The component is added to the Blocks overview page for future reference.

How to test

Visit https://calypso.live/devdocs/blocks/color-scheme-picker?branch=add/color-schemes/color-scheme-picker or test locally.
Navigate to the Blocks overview page and proceed to the ColorSchemePicker section.

Suggestions as to what to test

Does the component behave as expected?
Can the component easily be reused in the future?
Is the current responsive behavior sufficient?

Note

This PR currently compares its changes to PR #17157. This is so the changes can be easily identified. This PR will be pointed at master once PR #17157 has been merged. It also needs to be merged prior to PR #17259 which is based on this PR.

For a more detailed description of the feature, the chosen approach and its advantages feel free to check out my post on the subject: p4TIVU-75d-p2 (internal reference).

frontdevde · 2017-08-15T02:02:46Z

Note: I created the theme previews in Photoshop, that's why they are currently PNGs. I've already noted down converting them to SVGs as a future todo :)

frontdevde · 2017-08-16T23:45:34Z

/cc'ing @jonathansadowski @bluefuton @retrofox as GitHub suggests you as potential reviewers 🙂👋

spen

I think I may have a couple more points but I'll take a look with fresh 👀 tomorrow :)

The biggest blocker here would be the translation of the strings I think - so far the rest are just minor nitpicks.
Thanks for this work - looks awesome and can't wait to see it in the flesh!

spen · 2017-08-18T00:41:38Z

client/blocks/color-scheme-picker/README.md

+
+render() {
+	return (
+


blank line here

spen · 2017-08-18T00:49:43Z

client/blocks/color-scheme-picker/constants.js

+
+export const ColorSchemes = [
+	{
+		label: translate( 'Default' ),


I could be wrong here but I think there's some chance that calling translate early can cause issues. I think theres a chance that languages may not be loaded yet and so the passed in value will output as is.
I don't quite know the details but recently discussed this with @mcsf. Miguel, can you chime in here please?

Absolutely! The way I've done this in the past is wrapping translate calls in a function that takes a translate, and call that during the render phase, where we have this.props.translate. I found the same approach the other day in AppBanner:

wp-calypso/client/blocks/app-banner/utils.js

Lines 19 to 47 in 4c59897

export function getAppBannerData( translate, sectionName ) {

switch ( sectionName ) {

case EDITOR:

return {

title: translate( 'Rich mobile publishing.' ),

copy: translate( 'A streamlined editor with faster, simpler image uploading? Check and mate.' ),

};

case NOTES:

return {

title: translate( 'Watch engagement happening.' ),

copy: translate( 'Is your new post a hit? With push notifications, see reactions as they roll in.' ),

};

case READER:

return {

title: translate( 'Read online or off.' ),

copy: translate( 'Catch up with new posts when you have time, even if you are offline.' ),

};

case STATS:

return {

title: translate( 'Stats at your fingertips.' ),

copy: translate( "Add real-time stats to your device's notifications or widgets." ),

};

default:

return {

title: '',

copy: '',

};

}

}

@spen @mcsf Good point, thanks for the input! 👍

spen · 2017-08-18T01:20:51Z

client/blocks/color-scheme-picker/docs/example.jsx

+
+				{ this.state.selectedColorScheme &&
+					<Notice
+						text={ `Selected color scheme: ${ this.state.selectedColorScheme }` }


We should use the localize HoC and the translate prop it provides for this string.
I think it may end up looking a little like this:

... text={ translate( 'Selected color scheme: %s', { args: this.state.selectedColorScheme } ) }

I'm not sure if this will open up those values to translation, but if done right this could help us avoid using translate() in the constants file.

( More details on substituting strings in localize)

@spen Didn't add translate here as it's not part of the actual component but just an additional notice for the example in the docs to better visualize that a selection happened. Are we localizing strings like that in docs as well?

Good point, I think I'd just forgotten I was looking at an example for a moment! Since it is an example this looks perfect - thanks for correcting me ;)

spen · 2017-08-18T01:21:55Z

client/blocks/color-scheme-picker/docs/example.jsx

+		return (
+			<Card>
+				<ColorSchemePicker
+					temporarySelection={ true }


={ true } can be omitted here since it will be inferred by the prop being present without a value

<ColorSchemePicker temporarySelection

spen · 2017-08-18T01:22:58Z

client/blocks/color-scheme-picker/docs/example.jsx

+					<Notice
+						text={ `Selected color scheme: ${ this.state.selectedColorScheme }` }
+						showDismiss={ false }
+					/> }


Slight nit but can we bump this } down a line to stay inline with the opening {?

It was on its own line initially but got moved up by Calypso prettier. If that is still a requirement we'd either have to not run prettier on the file or the prettier config would need to support that. Given that it's a readability nice to have I assume it's OK to leave it this way?

Ahh yes :) I've not used prettier much so there'll likely be a few cases where I'm saying something counter to prettiers formatting... in those cases feel free to ignore :)

spen · 2017-08-18T01:29:34Z

client/blocks/color-scheme-picker/style.scss

+	background-image: url( '/calypso/images/color-schemes/theme-default.png' );
+
+	&.is-light-theme {
+


Nit: I think it'd be more in keeping with the rest of the stylesheets to remove the padded lines here and throughout this file.

spen · 2017-08-18T01:30:16Z

client/blocks/color-scheme-picker/index.jsx

+			<div className="color-scheme-picker">
+				<QueryPreferences />
+				<FormRadiosBar
+					isThumbnail={ true }


={ true } can be omitted here too.

mcsf · 2017-08-18T09:18:17Z

client/blocks/color-scheme-picker/index.jsx

+			colorSchemePreference: getPreference( state, 'colorScheme' ),
+		};
+	},
+	( dispatch, ownProps ) =>


Prefer ( dispatch, { temporarySelection } ) =>, so we can see at a glance which are necessary to ~~fetch state~~ compute action creators.

mcsf · 2017-08-18T09:19:32Z

client/blocks/color-scheme-picker/index.jsx

+				},
+			},
+			dispatch
+		)


~~bindActionCreators is seldom used in Calypso — passing an object of action creators to connect is the way we usually go, for simplicity.~~

^ scratch that one, I forgot it depended on temporarySelection

That said, it would be quite conceivable to achieve the same the following way:

connect( ( state ) => ( { colorSchemePreference: getPreference( state, 'colorScheme' ), } ), { saveColorSchemePreference } )( ColorSchemePicker ); // with const saveColorSchemePreference = ( preference, isTemporary ) => isTemporary ? setPreference( 'colorScheme', preference ) : savePreference( 'colorScheme', preference ); // and make sure `isTemporary` is passed within `handleColorSchemeSelection`

This is a situation where there are no strong project guidelines to choose one way over the other: there is a bit of personal taste, and a subjective perception of readability. I prefer the latter option, but this one is up to you. :-)

Yes, I'd agree that this approach would improve readability 👌

mcsf · 2017-08-18T09:23:55Z

client/blocks/color-scheme-picker/index.jsx

+		const addSelection = handleFunction => handleFunction( event.currentTarget.value );
+		temporarySelection && addSelection( onSelection );
+		addSelection( saveColorSchemePreference );
+	};


addSelection seems like an unnecessary abstraction. This would IMO read better and avoid an extra function instantiation if done more or less like:

const { value } = event.currentTarget; if ( temporarySelection ) { onSelection( value ); } saveColorSchemePreference( value );

Sure, let's go for the more readable approach 👍

mcsf · 2017-08-18T09:24:21Z

client/blocks/color-scheme-picker/index.jsx

+		saveColorSchemePreference: PropTypes.func,
+	};
+
+	handleColorSchemeSelection = event => {


Calypso style recommends wrapping all func args, so: ( event ) =>

@mcsf The parentheses were there initially but got removed by Calypso prettier. If that is still a requirement then I guess the prettier config would need to support that?

Ah, interesting. Maybe @samouri knows more?

I didn't realize that we recommend that! Is it in our style guidelines? I know that all of the examples use parens but I didn't think we prefer it one way or the other.

If we want to wrap all func args like that we can make the change to calypso-prettier

FWIW I think that having the parentheses makes it clearer, so I'd +1 making the change :)

frontdevde · 2017-08-18T13:31:50Z

@spen @mcsf Thanks for your input! I've added some comments and also pushed some updates based on your suggestions :)

spen

Awesome work @frontdevde :)
Thanks a heap for making the tweaks and clarifying on some of the other points!

Tests well and looks great in the devdocs 🎉

I have one more little request but it's not critical so accepting :)

spen · 2017-08-18T16:07:11Z

client/blocks/color-scheme-picker/README.md

+
+## Usage
+
+```es6


Tiny nit that I missed the first time around - could we change this to jsx please?
That'll give slightly better syntax highlighting in GH :)

Yep, already pushed ;)

folletto

Let's remove the <Notice> appearing when the option gets changed. The component should showcase only the component itself. If someone browses the documentation and sees the notice appear, would infer that the component includes the notice to, which is not the case.

It's important to show only the component in the component example, except if there are obvious limitations to do it.

folletto · 2017-08-21T18:01:26Z

Here you can find the images as SVGs (not minified, as it preserves the ID):
https://cloudup.com/cFktARQ9wSb

A consideration could be done here: it could inline the same SVG, and then style the color of each of the sub-components via CSS (the same CSS rules generated in the color scheme maybe?). Not required for this PR as it's better to have a simpler solution in, just as a general thought for later.

frontdevde · 2017-08-21T18:05:03Z

Thanks for your feedback, @folletto!

The idea behind the notice was to give some additional feedback as to which theme has been selected. But I'd agree that that could be misleading and thus just removed it from the docs as suggested.

Thanks, as well, for sending over the SVGs. Will add those in now 👍

frontdevde · 2017-08-21T18:36:27Z

OK, the SVGs are up as well. 👍 /cc @folletto

I like the idea of in the future using one as a template and styling them based on the color scheme styles themselves. Will add that to the list of potential future PRs/iterations.

folletto

Design is good, behaviour is good 👍

The SVG isn't exactly the same size, so it's slightly blurry, but let's review that later — it's good for now.

folletto · 2017-08-22T13:17:01Z

The SVG isn't exactly the same size, so it's slightly blurry, but let's review that later — it's good for now.

Fixed in i2: https://cloudup.com/c5afpz94OG6

frontdevde · 2017-08-22T13:46:33Z

Fixed in i2: https://cloudup.com/c5afpz94OG6

Thanks 👍 Just pushed a commit that updates the SVG assets.

frontdevde · 2017-08-22T18:59:58Z

I did an interactive rebase to squash the SVG commits into the initial commit in order to remove v1 of the SVGs as well as the PNGs from Git. /cc @spen

spen · 2017-08-23T00:28:07Z

I had another look over since the rebase (Thanks again for that!) and everything looks great!

I forgot to mention - I added the [Status] Blocked / Hold label to avoid this being merged mistakenly (since it relies on #17157).

frontdevde added the [Status] In Progress label Aug 15, 2017

frontdevde self-assigned this Aug 15, 2017

frontdevde requested a review from ehg August 15, 2017 01:53

matticbot added OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues labels Aug 15, 2017

frontdevde added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 15, 2017

spen self-requested a review August 15, 2017 04:55

This was referenced Aug 16, 2017

Color Schemes: Add Color Scheme Picker to Account Settings #17259

Merged

Color Schemes: Add new form components as basis for color scheme switcher #17157

Merged

frontdevde removed the [Status] In Progress label Aug 16, 2017

frontdevde mentioned this pull request Aug 17, 2017

Color Schemes: Add selected Color Scheme to Layout #17307

Merged

frontdevde added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Aug 17, 2017

frontdevde mentioned this pull request Aug 17, 2017

Color Schemes: Add Feature Detection #17310

Merged

spen suggested changes Aug 18, 2017

View reviewed changes

mcsf reviewed Aug 18, 2017

View reviewed changes

spen approved these changes Aug 18, 2017

View reviewed changes

frontdevde mentioned this pull request Aug 19, 2017

Color Schemes: Add color schemes via CSS custom properties #17366

Closed

spen added the [Status] Blocked / Hold label Aug 21, 2017

folletto suggested changes Aug 21, 2017

View reviewed changes

folletto approved these changes Aug 21, 2017

View reviewed changes

folletto removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Aug 21, 2017

Add Color Scheme Picker Component

5ecf925

frontdevde added 6 commits August 22, 2017 20:44

Add Color Scheme Picker Example to DevDocs Blocks

20ecd98

Move translate into component and address review suggestions

80fc637

Update formatting on Color Scheme Picker README

32703a6

Add jsx formatting to README

c7819af

Remove Notice from Docs Example

8635a57

Update color scheme thumbnail references

25c9550

frontdevde force-pushed the add/color-schemes/color-scheme-picker branch from 7d5578b to 25c9550 Compare August 22, 2017 18:54

spen removed the [Status] Blocked / Hold label Aug 30, 2017

spen merged commit 023b8cd into add/color-schemes/form-components Aug 30, 2017

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 30, 2017

This was referenced Aug 30, 2017

Color Schemes: Add Color Scheme Picker II #17641

Closed

Color Schemes: Add Color Scheme Picker II #17642

Closed

Color Schemes: Add Color Scheme Picker II #17644

Merged

alisterscott deleted the add/color-schemes/color-scheme-picker branch February 1, 2018 06:36

designsimply added [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. DevDocs and removed [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. labels Feb 12, 2018

bluefuton mentioned this pull request Feb 26, 2018

Reader: when using Light color scheme, gray fade is visible on tag remove icons #22796

Closed

	export function getAppBannerData( translate, sectionName ) {
	switch ( sectionName ) {
	case EDITOR:
	return {
	title: translate( 'Rich mobile publishing.' ),
	copy: translate( 'A streamlined editor with faster, simpler image uploading? Check and mate.' ),
	};
	case NOTES:
	return {
	title: translate( 'Watch engagement happening.' ),
	copy: translate( 'Is your new post a hit? With push notifications, see reactions as they roll in.' ),
	};
	case READER:
	return {
	title: translate( 'Read online or off.' ),
	copy: translate( 'Catch up with new posts when you have time, even if you are offline.' ),
	};
	case STATS:
	return {
	title: translate( 'Stats at your fingertips.' ),
	copy: translate( "Add real-time stats to your device's notifications or widgets." ),
	};
	default:
	return {
	title: '',
	copy: '',
	};
	}
	}

		background-image: url( '/calypso/images/color-schemes/theme-default.png' );

		&.is-light-theme {


		render() {
		return (


		## Usage

		```es6

Color Schemes: Add Color Scheme Picker #17200

Color Schemes: Add Color Scheme Picker #17200

Conversation

frontdevde commented Aug 15, 2017 • edited by folletto Loading

Notable additions in this PR:

How to test

Suggestions as to what to test

Note

frontdevde commented Aug 15, 2017

frontdevde commented Aug 16, 2017

spen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

mcsf Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

mcsf Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frontdevde commented Aug 18, 2017

spen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

folletto left a comment

Choose a reason for hiding this comment

folletto commented Aug 21, 2017

frontdevde commented Aug 21, 2017

frontdevde commented Aug 21, 2017

folletto left a comment

Choose a reason for hiding this comment

folletto commented Aug 22, 2017

frontdevde commented Aug 22, 2017

frontdevde commented Aug 22, 2017 • edited Loading

spen commented Aug 23, 2017

frontdevde commented Aug 15, 2017 •

edited by folletto

Loading

mcsf Aug 18, 2017 •

edited

Loading

mcsf Aug 18, 2017 •

edited

Loading

mcsf Aug 18, 2017 •

edited

Loading

frontdevde commented Aug 22, 2017 •

edited

Loading