Chrome: Persist the state of the sidebar across page refresh #2462

youknowriad · 2017-08-18T11:02:23Z

This PR bootstrap the work to persist some UI preferences locally. (It uses localStorage)
For now, this persists only the sidebar state (opened/closed) but could be updated later to include the state of the difference sidebar panels.

solves #450 partially

codecov · 2017-08-18T11:10:33Z

Codecov Report

Merging #2462 into master will increase coverage by 0.39%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #2462      +/-   ##
==========================================
+ Coverage   26.98%   27.37%   +0.39%     
==========================================
  Files         159      161       +2     
  Lines        4903     4968      +65     
  Branches      817      830      +13     
==========================================
+ Hits         1323     1360      +37     
- Misses       3035     3055      +20     
- Partials      545      553       +8

Impacted Files	Coverage Δ
editor/index.js	`0% <ø> (ø)`	⬆️
editor/store-persist.js	`100% <100%> (ø)`
editor/selectors.js	`96.32% <100%> (+0.08%)`	⬆️
editor/reducer.js	`88.73% <100%> (ø)`
editor/store.js	`83.33% <83.33%> (ø)`
blocks/inspector-controls/range-control/index.js	`83.33% <0%> (-16.67%)`	⬇️
blocks/library/paragraph/index.js	`38.46% <0%> (-8.6%)`	⬇️
blocks/editable/tinymce.js	`0% <0%> (ø)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccf991a...c932c96. Read the comment docs.

jasmussen · 2017-08-18T11:22:08Z

Hoooraaay! It's like my birthday! I love love this, and it works great!

aduth · 2017-08-18T18:49:21Z

editor/selectors.js

 * Returns true if the editor sidebar panel is open, or false otherwise.
 *
 * @param  {Object}  state Global application state
 * @return {Boolean}       Whether sidebar is open
 */
 export function isEditorSidebarOpened( state ) {
-	return state.isSidebarOpened;
+	return state.preferences.isSidebarOpened;


Minor: Should this compose from getPreferences, i.e. return getPreferences( state ).isSidebarOpened; ? Should we have a getPreference selector? Maybe I'm being premature? 😄

aduth · 2017-08-18T18:49:45Z

editor/store.js

@@ -0,0 +1,64 @@
+/**


Thanks for splitting off this file. I've been wanting to separate reducers from store initialization for a while 👍

aduth · 2017-08-18T18:54:41Z

editor/store.js

+ * Loads the initial preferences and saves them to the local storage once changed
+ * @param {Object} store Redux Store
+ */
+function setupStorePersistence( store ) {


Again, maybe getting ahead of myself, but thinking about how this might be implemented for maintainability. Redux Persist seems to be the canonical reference here, though I think it's overkill for what we need. That said, maybe their patterns of reducer key whitelisting and rehydration actions could serve as good inspiration?

Also, this seems to fall under the pattern of what I'd consider to be a store enhancer: http://redux.js.org/docs/api/createStore.html . It could fit well within what we're already creating as an enhancers array below (a store enhancer accepts createStore and returns a new createStore, example).

Can we test this behavior? Seems like it wouldn't be too hard to mock/spy localStorage.

Absolutely, thanks for mentioning this. I extracted the behaviour into a generic simple store enhancer allowing to persist and autohydrate a specific reducer key.

It could be easily extended later (or replaced by redux persist if needed).

aduth · 2017-08-18T18:55:31Z

editor/test/reducer.js

+
+		it( 'should update preferences', () => {
+			const prefs = { awesome: true };
+			const state = preferences( { isSidebarOpened: false }, {


Should we apply deepFreeze to original state to guard against mutations?

aduth · 2017-08-18T19:02:11Z

Should userData reducer be collapsed into preferences as well, so we can remember recent blocks?

aduth · 2017-08-18T19:02:58Z

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

youknowriad · 2017-08-21T11:09:01Z

Should userData reducer be collapsed into preferences as well, so we can remember recent blocks?

Let's keep this PR as a bootstrap PR for preferences persistence and leave this for later. Also thinking about using this to store the open/closed status of the sidebar panels.

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

Yes probably but AFAIK this endpoint does not exist yet (see #1455). Local storage is an acceptable v1 for me.

aduth · 2017-08-21T12:24:11Z

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

Yes probably but AFAIK this endpoint does not exist yet (see #1455). Local storage is an acceptable v1 for me.

Sorry, I should have avoided the word API as it's overused 😄 I meant the window.setUserSetting method which tracks to localStorage intended for preferences like these. I believe it also eventually persists to the database to bootstrap after being cleared. Also probably manages the case where user logs out and another logs in (those settings should be cleared, or switched to the new user).

It was previously used in the stats tracking code: https://github.com/WordPress/gutenberg/pull/2140/files#diff-17b4c77ddf8f9543bfb9ff3e64b0a6dbR23

youknowriad · 2017-08-21T12:29:28Z

Sorry, I should have avoided the word API as it's overused

:) I was not aware of these functions. Should we declare a script dependency or we just assume these functions are available for WP code?

youknowriad · 2017-08-21T12:42:19Z

@aduth It looks like we can't use this API to store complex JSON object. It's for scalar values only. If I use JSON.stringify/JSON.parse, it looks like the string is sanitized or something.

aduth · 2017-08-21T14:54:01Z

@aduth It looks like we can't use this API to store complex JSON object. It's for scalar values only. If I use JSON.stringify/JSON.parse, it looks like the string is sanitized or something.

Yeah, digging into it a bit more, I don't know that the setUserSettings method is appropriate for this at the moment. Specifically, the current implementation works off of cookies, which don't allow for much storage:

https://github.com/WordPress/wordpress-develop/blob/7157569/src/wp-includes/js/utils.js#L151-L184

I'm wondering if it might be worth pursuing (separately) some improvements to this function to use either localStorage or indexedDB, and to accept more than simple scalar values for storage.

In the meantime, I think it would be appropriate to stick to localStorage, though I'm still concerned about the "logout" case. Do you have any ideas for how we could address this, here or separately?

youknowriad · 2017-08-21T15:00:52Z

In the meantime, I think it would be appropriate to stick to localStorage, though I'm still concerned about the "logout" case. Do you have any ideas for how we could address this, here or separately?

An option would be to add a "logout" hook and clear the localStorage key, the only concern is the duplication of the storage key constant in PHP.

youknowriad · 2017-08-22T09:40:29Z

@aduth It looks like the User Settings API rely on a global userSettings.uid provided by WP to ensure uniqueness. So I pushed a simple fix appending this global to the storage key.

aduth

Looks good 👍

aduth · 2017-08-22T12:14:21Z

editor/store-persist.js

+			const newStateValue = store.getState()[ reducerKey ];
+			if ( newStateValue !== currentStateValue ) {
+				currentStateValue = newStateValue;
+				window.localStorage.setItem( storageKey, JSON.stringify( currentStateValue ) );


Currently this is probably fine since we're not expecting the specific state value to change very often, but this might be something worth debouncing or throttling in the future.

jasmussen · 2017-08-22T12:17:37Z

🎉💓💓💓🤘

aduth · 2017-08-28T19:57:57Z

Should mobile be exempt from this persistence? Noting while switching to emulation that the sidebar being opened sticks across page loads for mobile, which while unlikely to occur in real-world usage seems odd (i.e. open sidebar on mobile, refresh page, would expect sidebar to be closed).

jasmussen · 2017-09-25T08:57:38Z

(i.e. open sidebar on mobile, refresh page, would expect sidebar to be closed)

I agree with 👆

youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label Aug 18, 2017

youknowriad self-assigned this Aug 18, 2017

youknowriad requested review from mtias, jasmussen and aduth August 18, 2017 11:02

aduth reviewed Aug 18, 2017

View reviewed changes

youknowriad force-pushed the add/persist-preferences branch from 1b6aaf0 to 196a284 Compare August 21, 2017 11:05

youknowriad added 5 commits August 22, 2017 10:20

Chrome: Persist the state of the sidebar across page refresh

9d81785

Chrome: Add getPreference Selector

01bb506

Tests: deep freeze original state to ensure puriness

e63dc0d

Chrome: Extract the persistence store enhancer to its own generic file

60f0e0a

Chrome: Per user local preferences

f17cbd0

youknowriad force-pushed the add/persist-preferences branch from 196a284 to f17cbd0 Compare August 22, 2017 09:39

youknowriad mentioned this pull request Aug 22, 2017

Add user preference api #1948

Closed

aduth added 2 commits August 22, 2017 08:07

Framework: Split store enhancers per line

2aa20eb

Framework: Fix typo "persisted"

c932c96

aduth approved these changes Aug 22, 2017

View reviewed changes

youknowriad merged commit 3c6bd72 into master Aug 22, 2017

aduth deleted the add/persist-preferences branch August 28, 2017 19:57

This was referenced Sep 27, 2017

Bump plugin to version 1.2.0 #2810

Merged

Chrome: Exempt mobile from sidebar toggled state persistence #2816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chrome: Persist the state of the sidebar across page refresh #2462

Chrome: Persist the state of the sidebar across page refresh #2462

youknowriad commented Aug 18, 2017 •

edited

Loading

codecov bot commented Aug 18, 2017 •

edited

Loading

jasmussen commented Aug 18, 2017

aduth Aug 18, 2017

youknowriad Aug 21, 2017

aduth Aug 18, 2017

aduth Aug 18, 2017

aduth Aug 18, 2017

youknowriad Aug 21, 2017

aduth Aug 18, 2017

aduth commented Aug 18, 2017

aduth commented Aug 18, 2017

youknowriad commented Aug 21, 2017

aduth commented Aug 21, 2017

youknowriad commented Aug 21, 2017 •

edited

Loading

youknowriad commented Aug 21, 2017

aduth commented Aug 21, 2017

youknowriad commented Aug 21, 2017

youknowriad commented Aug 22, 2017

aduth left a comment

aduth Aug 22, 2017

jasmussen commented Aug 22, 2017

aduth commented Aug 28, 2017

jasmussen commented Sep 25, 2017

Chrome: Persist the state of the sidebar across page refresh #2462

Chrome: Persist the state of the sidebar across page refresh #2462

Conversation

youknowriad commented Aug 18, 2017 • edited Loading

codecov bot commented Aug 18, 2017 • edited Loading

Codecov Report

jasmussen commented Aug 18, 2017

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

aduth commented Aug 18, 2017

aduth commented Aug 18, 2017

youknowriad commented Aug 21, 2017

aduth commented Aug 21, 2017

youknowriad commented Aug 21, 2017 • edited Loading

youknowriad commented Aug 21, 2017

aduth commented Aug 21, 2017

youknowriad commented Aug 21, 2017

youknowriad commented Aug 22, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Aug 22, 2017

aduth commented Aug 28, 2017

jasmussen commented Sep 25, 2017

youknowriad commented Aug 18, 2017 •

edited

Loading

codecov bot commented Aug 18, 2017 •

edited

Loading

youknowriad commented Aug 21, 2017 •

edited

Loading