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.

Sign up for GitHub

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

Jump to bottom

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

Merged

spen merged 8 commits into master from add/color-schemes/account-me-integration

Sep 7, 2017

Contributor

frontdevde commented Aug 16, 2017 •

edited

Loading

This PR is part of a series of PRs aiming to provide support for Color Schemes in Calypso.
This PR focuses on integrating the Color Scheme Picker (PR #17200) in the Account Settings.

Notable additions in this PR:

This PR adds support for nested settings to UserSettings (/lib/user-settings).
The feature flag 'me/account/color-scheme-picker' is introduced.
The ColorSchemePicker component is added to the me/account/ page.

Considerations

The ColorSchemePicker by default connects to/via state/preferences/. I initially contemplated using the same approach for the integration on me/account/ but during development realized how deeply integrated with /lib/user-settings the whole page is (via form-base). In order to not having to duplicate all the functionality on that page, I then decided to add support for nested settings to /lib/user-settings instead. This way there's hardly any overhead involved in integrating the ColorSchemePicker on the Account Settings page.

How to test

Visit https://calypso.live/?branch=add/color-schemes/account-me-integration or test locally.
Navigate to the Accounts Settings page and scroll down to the Admin Color Scheme section.

Suggestions as to what to test

Is the color scheme selection being persisted when saved and dropped when not?
Do all combinations of different Account Settings changes work as expected?
Double check other parts of the app making use of (/lib/user-settings).

Note

This PR currently compares its changes to PR #17200 which in turn compares to PR #17157. This is so the changes can be easily identified. This PR will be pointed at master once PR #17200 and #17157 have been merged.

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 added the [Status] In Progress label

frontdevde self-assigned this

frontdevde requested a review from ehg

August 16, 2017 23:03

matticbot added OSS Citizen [Size] L labels

frontdevde mentioned this pull request

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

Merged

frontdevde added [Status] Needs Review and removed [Status] In Progress labels

frontdevde requested a review from spen

August 16, 2017 23:37

This was referenced Aug 16, 2017

Color Schemes: Add Color Scheme Picker #17200

Merged

Color Schemes: Add selected Color Scheme to Layout #17307

Merged

frontdevde added the [Status] Needs Design Review label

frontdevde mentioned this pull request

Color Schemes: Add Feature Detection #17310

Merged

frontdevde added the [Feature] User & Account Settings (/me) label

frontdevde mentioned this pull request

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

Closed

Contributor

folletto commented Aug 21, 2017

Design wise it's good.

In the future we'll have a separate section for this, but as a first PR this is good: 👍

folletto removed the [Status] Needs Design Review label

matticbot added the [Status] Needs Rebase label

Automattic deleted a comment from matticbot

frontdevde removed the [Status] Needs Rebase label

frontdevde force-pushed the add/color-schemes/color-scheme-picker branch from 7d5578b to 25c9550 Compare

August 22, 2017 18:54

spen added the [Status] Blocked / Hold label

Contributor

spen commented Aug 23, 2017

Adding the [Status] Blocked / Hold label until #17200 is merged to master :)

spen reviewed

View reviewed changes

client/lib/user-settings/index.js Outdated

@@ @@ -1,16 +1,9 @@ @@
               /**
                * External dependencies
                */
+              import { assign, isEmpty, keys, merge, has, get, set, unset } from 'lodash';

Contributor

spen Aug 18, 2017

❤️

Contributor Author

frontdevde Aug 23, 2017

Before I comment on the other comments below:
Thanks for your review, highly appreciated! 💯

client/lib/user-settings/index.js

+              		unset( settings, settingName );
+              		const settingKeys = settingName.split( '.' );
+              		if ( settingKeys.length > 1 ) {

Contributor

spen Aug 23, 2017

What do you think to using ! isEmpty( settingKeys ) here? I feel that would be in keeping with the rest of the checks in this function.

Contributor Author

frontdevde Aug 23, 2017

Hmm, ! isEmpty( settingKeys ) would be true though even if there is only one setting key. We only want to continue if there is more than one key e.g. ["calypso_settings", "colorScheme"].

Contributor

spen Aug 23, 2017

Ahh my bad! For some reason I was reading/treating this as settingKeys.length >= 1!

client/lib/user-settings/index.js

+              			const parentKey = settingKeys.join( '.' );
+              			//if parent is empty, call function again
+              			if ( parentKey && isEmpty( get( settings, parentKey ) ) ) {
+              				deleteUnsavedSetting( settings, parentKey, true );

Contributor

spen Aug 23, 2017

It might be because it's late, but I think I'm missing the need to recurse here - is this to cover strings that represent nested settings, similar to _.gets usage?
So something like the following would work?:

deleteUnsavedSetting( userSettings, 'preferences.colorTheme' )
console.log( userSettings.preferences.colorTheme );
// undefined

Or something else?

Contributor Author

frontdevde Aug 23, 2017

Yes, the idea behind the update to user settings is to support nested settings like 'preferences.colorTheme'. The current way of deletion only deleted the key but resulted in an empty parent key in this.unsavedSettings. As hasUnsavedUserSettings only checks for isEmpty, it would return true despite no settings being unsaved. Therefore we have to ensure to remove potentially empty parent keys.

e.g. {calypso_preferences: { colorScheme: "dark" }
=> {calypso_preferences: { } }
=> { }

client/lib/user-settings/index.js Outdated

                */
               UserSettings.prototype.getOriginalSetting = function( settingName ) {
               	var setting = null;
-              	if ( this.settings && this.settings[ settingName ] ) {
-              		setting = this.settings[ settingName ];
+              	if ( this.settings && get( this.settings, settingName ) ) {

Contributor

spen Aug 23, 2017 •

edited

Loading

This may be outside the scope of this PR but I believe theres an issue here (before & after the change).
I think we could quite easily fix that here though, if you don't mind?

Here's a quick demo showing the issue... basically any falsey value would be replaced with null:

https://runkit.com/spen/599cd633888c48001253cc1d

I think something like so should do the trick:

getOriginalSetting = settingName =>
  has( this.settings, settingName )
    ? get( this.settings, settingName )
    : null;

Contributor Author

frontdevde Aug 30, 2017

Commenting for continuity: As per our discussion in Slack, we'll address this one outside the scope of this PR in order to isolate any potential side effects.

Contributor

spen Aug 30, 2017

Just a note on the code example I gave above - that can be squished down further:

getOriginalSetting = settingName => get( this.settings, settingName, null );

client/lib/user-settings/index.js Outdated

-              		setting = ( 'undefined' !== typeof this.unsavedSettings[ settingName ] )
-              			? this.unsavedSettings[ settingName ]
-              			: this.settings[ settingName ];
+              	if ( this.settings && 'undefined' !== typeof get( this.settings, settingName ) ) {

Contributor

spen Aug 23, 2017

Just a suggestion here, FFTI as it's not necessarily in scope for this PR...

_.isUndefined could be useful here, but I think _.has would be even more useful.

If IIRC we could squish these checks down like so:

if ( this.settings && 'undefined' !== typeof get( this.settings, settingName ) ) {
// >>
if ( ! has( this.settings, settingName ) ) {

Contributor Author

frontdevde Aug 23, 2017

Yes, that should work. 👍
Just without the !, as it's checking for not undefined:
if ( has( this.settings, settingName ) ) {

client/lib/user-settings/index.js

    
              			'user_login' !== settingName

              		) {

              			debug( 'Removing ' + settingName + ' from changed settings.' );

              			delete this.unsavedSettings[ settingName ];

              			deleteUnsavedSetting( this.unsavedSettings, settingName );

Contributor

spen Aug 23, 2017

I'm a fan of the functional-izing here 🙂 👍

client/lib/user-settings/index.js Outdated

-              	if ( settingName in this.unsavedSettings ) {
-              		delete this.unsavedSettings[ settingName ];
+              	if ( has( this.unsavedSettings, settingName ) ) {

Contributor

spen Aug 23, 2017 •

edited

Loading

This check might also be a candidate for this.isSettingUnsaved.

spen reviewed

View reviewed changes

client/lib/user-settings/index.js Outdated

-              			: this.settings[ settingName ];
+              	if ( this.settings && 'undefined' !== typeof get( this.settings, settingName ) ) {
+              		setting =
+              			'undefined' !== typeof get( this.unsavedSettings, settingName )

Contributor

spen Aug 23, 2017

I could be wrong here, but we may be able to use this.isSettingUnsaved here - if so that might help with readability :)

spen reviewed

View reviewed changes

client/me/account/main.jsx

		@@ -529,6 +534,14 @@ const Account = React.createClass( {

		{ this.communityTranslator() }

		{ config.isEnabled( 'me/account/color-scheme-picker' ) &&

Contributor

spen Aug 30, 2017

I see that we're only enabling in development in this PR, so this is not a huge issue but to err on the side of caution what do you think to merging #17310 first and including the check in this PR? (See: #17310 (review))

Contributor Author

frontdevde Aug 30, 2017

Commenting for continuity: As per our discussion in Slack just now, we're going to leave the PR order as is to ensure we have a good test case for the feature detection in #17310. We're aiming to merge the PRs in quick succession.

frontdevde changed the base branch from add/color-schemes/color-scheme-picker to master

August 30, 2017 17:32

Contributor

matticbot commented Aug 30, 2017

@frontdevde This PR needs a rebase

matticbot added the [Status] Needs Rebase label

spen removed the [Status] Blocked / Hold label

frontdevde force-pushed the add/color-schemes/account-me-integration branch from 0608b5a to c2fa9e4 Compare

August 30, 2017 17:53

matticbot added [Size] M and removed [Size] L labels

frontdevde removed the [Status] Needs Rebase label

matticbot added [Size] L [Status] Needs Rebase and removed [Size] M labels

Contributor

matticbot commented Sep 4, 2017

@frontdevde This PR needs a rebase

frontdevde force-pushed the add/color-schemes/account-me-integration branch from 3ec9837 to abcd2df Compare

September 4, 2017 15:54

frontdevde removed the [Status] Needs Rebase label

spen approved these changes

View reviewed changes

Contributor

spen left a comment

Looking great :D

client/me/account/main.jsx Outdated

-              		this.updateUserSetting( 'calypso_preferences.colorScheme', colorScheme );
+              		const settingName = 'calypso_preferences.colorScheme';
+              		// set a fallback color scheme if no default value is provided by the API

Contributor

spen Sep 5, 2017

I'd be tempted to mention that this is a bit of a hack, allowing us to use this.props.userSettings.updateSetting without an existing value and mention the behaviour we see without forcing this default.
In the TODO, it may also be worth explicitly mentioning that the intention is to remove this line.
What do you think?

Contributor Author

frontdevde Sep 6, 2017

Done 👍

client/me/account/main.jsx Outdated

+              		// set a fallback color scheme if no default value is provided by the API
+              		// TODO: ideally the default value should be provided on the API side
+              		update( this.props.userSettings.settings, settingName, value => value || 'default' );

Contributor

spen Sep 6, 2017

This seems to do the trick! I'm seeing the button become active when making selections, then the option saved is persisted :)

frontdevde force-pushed the add/color-schemes/account-me-integration branch from abcd2df to 29c0814 Compare

September 6, 2017 12:27

spen approved these changes

View reviewed changes

client/me/account/main.jsx

+              	updateColorScheme( colorScheme ) {
+              		const settingName = 'calypso_preferences.colorScheme';
+              		// Set a fallback color scheme if no default value is provided by the API.

Contributor

spen Sep 6, 2017

👍 👍 👍 :)

Contributor

matticbot commented Sep 6, 2017

@frontdevde This PR needs a rebase

matticbot added the [Status] Needs Rebase label

frontdevde added 8 commits

September 7, 2017 12:23


          Add support for nested settings to UserSettings

1a0b09d


          Add Color Scheme Picker to me/account/

b0716f2


          Update formatting on Account Settings

f8445a7


          Use isSettingUnsaved where appropriate

67b2fb4


          Improve readability of prop check in updateSetting

77e4c3c


          Improve readability of prop check in getSetting

8f1e84d


          Add lodash deps

031a4a5


          Set a fallback color scheme if no default value is provided

05a43d9

frontdevde force-pushed the add/color-schemes/account-me-integration branch from 29c0814 to 05a43d9 Compare

September 7, 2017 10:25

frontdevde removed the [Status] Needs Rebase label

spen merged commit 98e98d1 into master

matticbot removed the [Status] Needs Review label

alisterscott deleted the add/color-schemes/account-me-integration branch

February 1, 2018 06:37

bperson mentioned this pull request

Fix user settings deleting unsaved settings behavior when coming back to the original values #23232

Merged

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

Labels

[Feature] User & Account Settings (/me) OSS Citizen [Size] L