Move preferences to redux #5401

artpi · 2016-05-16T19:46:48Z

Moves Preferences data to redux, as listed in #5046

Introduces preferences redux tree
Introduces QueryPreferences component
Uses this in post-editor

Nice and shiny readme

Motivation

We need it for offline - its in the editor
I want to create calypso-labs to let users enable experimental features and need a place to store data

Testing

Only preference moved is editor mode, so:

while editing, change editor mode to html
close window
clear localstorage
open new window, go to post editing and see if the mode is html

CC @rralian @gwwar @mtias @aduth @retrofox

aduth · 2016-05-17T16:41:40Z

client/post-editor/post-editor.jsx

-				this.recordedDefaultEditorMode = true;
-			}
+		const editorMode = this.props.editorModePreference || 'tinymce';
+		if ( ! this.recordedDefaultEditorMode ) {


This is not quite the same effect as existed previously, as before we had guarded against recording the default mode until after preferences have loaded. With this refactor, we can't guarantee that the recorded mode is in-fact the user preference, as it may simply be the fallback value while preferences are loading.

Thanks! That logic was very well hidden! I rewrite it a bit to behave in the same way and maybe a bit clearer.

gwwar · 2016-05-17T21:54:29Z

client/state/preferences/reducer.js

+		case PREFERENCES_RECEIVE:
+			return false;
+		case PREFERENCES_FETCH:
+			return true;


Don't forget to not persist this

case SERIALIZE: case DESERIALIZE: return false;

aduth · 2016-05-19T12:09:54Z

We should also be sure to remove the <PreferencesData /> rendering from post-editor/controller.js and switch the <PostEditor /> preferences propType with editorModePreference.

artpi · 2016-05-20T13:09:36Z

@aduth @gwwar thanks for taking the time! I implemented your suggestions and now I will write proper tests.

We should also be sure to remove the...

done.

artpi · 2016-05-20T13:22:28Z

Also, I think I will move this in following direction:

Instead of having generic key/value store for options, I will introduce dedicated reducer for each one. That way, the shape will be more clear. Also will solve schema problems for serialization.
Move bumpstat in loading default editor mode to analytics middleware, thus cleaning up post editor/
Also: tests.

gwwar · 2016-06-08T17:44:13Z

client/state/preferences/test/reducer.js

+		} );
+
+		it( 'should use DEFAULT_PREFERENCES from constants as a default', () => {
+			expect( reducer( undefined, {} ).values ).to.deep.equal( DEFAULT_PREFERENCES );


.eql is deep equal, and .equal checks for reference equality

gwwar · 2016-06-08T18:23:56Z

👍 I took this out for a spin and it works well. Ping me when you add a schema in and I can retest persistence. We should be able to after that makes it in.

aduth · 2016-06-08T18:30:31Z

Not sure if we have a particular timeline for this, but I'd really like to start investigating some improvements to how we accept preferences in our REST API generally, especially in mind of some of the hacks we've needed to incorporate (including entire preferences payload in update) and to address 2FA authorization stale issues (#202).

I guess this may not be a blocker for the pull request, since as long as the action creator and selectors signatures continue to match our intended direction (single preference updating and retrieval) it won't be difficult to change the underlying request/storage behavior.

Ref: 600-gh-io

gwwar · 2016-06-08T18:41:25Z

especially in mind of some of the hacks we've needed to incorporate (including entire preferences payload in update)

These issues are out of scope for this PR but +1 for fixing these in follow up PRs. Do we have issues open for some of the API improvements?

gziolo · 2016-06-09T06:33:16Z

client/state/preferences/reducer.js

+
+const values = combineReducers( mapValues( DEFAULT_PREFERENCES,
+	( value, key ) => createReducerForPreferenceKey( key, value )
+) );


Very interesting usage :D

aduth · 2016-06-09T12:12:12Z

So I'd taken for granted our discussion about the need for getState being based on the need to pass the entire settings payload when updating a single value. But I investigated this more and this is not true. You can pass a single key-value pair object and the other preference keys will not be affected. With this in mind, I think we should revise our setPreference action creator to avoid getState.

Past discussion:

@aduth

…be possible thanes to @aduth discovery

artpi · 2016-06-09T14:55:37Z

@aduth : thanks for investigating!

With this in mind, I think we should revise

Done.

gwwar · 2016-06-09T16:18:39Z

👍 Looks good to me. Don't forget to squash. 😄

aduth · 2016-06-09T17:42:46Z

Hmm, testing this live, it doesn't seem that the change in preference takes effect when switching editor tabs after the first refresh. It takes a second refresh in order to stick.

aduth · 2016-06-09T17:47:42Z

client/post-editor/post-editor.jsx

@@ -884,7 +886,9 @@ export default connect(
 			receivePost,
 			editPost,
 			resetPostEdits,
-			setEditorPostId
+			setEditorPostId,
+			setPreference,


Not using setPreference prop?

mtias · 2016-06-10T13:42:34Z

@artpi reloading a page seems to get rid of my recently selected sites.

artpi · 2016-06-10T17:52:56Z

reloading a page seems to get rid of my recently selected sites.

That seems unrelated, I did not touch recentSites, I only introduced a framework.

artpi self-assigned this May 16, 2016

artpi added [Status] In Progress State labels May 16, 2016

artpi added this to the Offline i1: State tree consolidation milestone May 16, 2016

mtias mentioned this pull request May 16, 2016

Framework: Redux single data tree and query components #5046

Closed

artpi added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 17, 2016

aduth reviewed May 17, 2016
View reviewed changes

artpi removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 17, 2016

gwwar reviewed May 17, 2016
View reviewed changes

artpi force-pushed the new/redux-preferences branch 2 times, most recently from 5b5e5dd to 4d297b0 Compare June 8, 2016 12:09

artpi added 3 commits June 8, 2016 15:17

Initial commit

55f2107

Proper actions and reducers

1f847a0

Add queryComponent fir preferences

109d49b

gwwar reviewed Jun 8, 2016
View reviewed changes

gziolo reviewed Jun 9, 2016
View reviewed changes

artpi added 4 commits June 9, 2016 13:18

Alphabetize action-types

ebcfece

Space after QueryPreferences

77fb4f9

Get rid of recordedDefaultEditorModeFromPreferences

52e9de0

fix typo in readme

3a5add9

artpi added 4 commits June 9, 2016 14:43

Fix tests

53f0939

remove extra import

11c7f21

Add comment explaining why saving prefernces work that way

01511e8

Get rid of getState frim thunk and save settings individually.It can …

5950898

…be possible thanes to @aduth discovery

Add tests for failing action

7cba2d5

gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 9, 2016

artpi merged commit a091318 into master Jun 9, 2016

matticbot removed the [Status] Ready to Merge label Jun 9, 2016

aduth reviewed Jun 9, 2016
View reviewed changes

artpi mentioned this pull request Jun 17, 2016

Remove sessionStorage, mediaModalGalleryInstructionsDismissed -> redux #6096

Merged

mtias deleted the new/redux-preferences branch June 27, 2016 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move preferences to redux #5401

Move preferences to redux #5401

artpi commented May 16, 2016 •

edited

Loading

aduth May 17, 2016

artpi May 19, 2016

gwwar May 17, 2016 •

edited

Loading

artpi May 19, 2016

aduth commented May 19, 2016

artpi commented May 20, 2016 •

edited

Loading

artpi commented May 20, 2016 •

edited

Loading

gwwar Jun 8, 2016

gwwar commented Jun 8, 2016

aduth commented Jun 8, 2016

gwwar commented Jun 8, 2016 •

edited

Loading

gziolo Jun 9, 2016

aduth commented Jun 9, 2016

artpi commented Jun 9, 2016

gwwar commented Jun 9, 2016

aduth commented Jun 9, 2016

aduth Jun 9, 2016

mtias commented Jun 10, 2016

artpi commented Jun 10, 2016

Move preferences to redux #5401

Move preferences to redux #5401

Conversation

artpi commented May 16, 2016 • edited Loading

Motivation

Testing

aduth May 17, 2016

Choose a reason for hiding this comment

artpi May 19, 2016

Choose a reason for hiding this comment

gwwar May 17, 2016 • edited Loading

Choose a reason for hiding this comment

artpi May 19, 2016

Choose a reason for hiding this comment

aduth commented May 19, 2016

artpi commented May 20, 2016 • edited Loading

artpi commented May 20, 2016 • edited Loading

gwwar Jun 8, 2016

Choose a reason for hiding this comment

gwwar commented Jun 8, 2016

aduth commented Jun 8, 2016

gwwar commented Jun 8, 2016 • edited Loading

gziolo Jun 9, 2016

Choose a reason for hiding this comment

aduth commented Jun 9, 2016

artpi commented Jun 9, 2016

gwwar commented Jun 9, 2016

aduth commented Jun 9, 2016

aduth Jun 9, 2016

Choose a reason for hiding this comment

mtias commented Jun 10, 2016

artpi commented Jun 10, 2016

artpi commented May 16, 2016 •

edited

Loading

gwwar May 17, 2016 •

edited

Loading

artpi commented May 20, 2016 •

edited

Loading

artpi commented May 20, 2016 •

edited

Loading

gwwar commented Jun 8, 2016 •

edited

Loading