Don't load isSidebarOpened preference from localStorage on mobile. #3331

jorgefilipecosta · 2017-11-03T15:28:20Z

Description

This PR aims to close #2816. When loading preferences from the localStorage if we are on mobile it sets isSidebarOpened to false instead of using what is stored. The technique used to define if we are on mobile is the same that is already used to set the default value for the isSidebarOpened when not loading from localStorage.

Testing

Resize your window on to mobile sizes (or test on mobile). Open sidebar if it is not already open. Refresh the page and see sidebar is now closed.
Verify that other preferences are still persisted e.g visual/text mode.

codecov · 2017-11-03T15:28:23Z

Codecov Report

Merging #3331 into master will increase coverage by 0.08%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #3331      +/-   ##
==========================================
+ Coverage   34.85%   34.93%   +0.08%     
==========================================
  Files         261      263       +2     
  Lines        6717     6730      +13     
  Branches     1224     1228       +4     
==========================================
+ Hits         2341     2351      +10     
- Misses       3692     3694       +2     
- Partials      684      685       +1

Impacted Files	Coverage Δ
editor/store.js	`83.33% <ø> (ø)`	⬆️
editor/constants.js	`100% <100%> (ø)`
editor/utils/mobile/index.js	`57.14% <57.14%> (ø)`

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 804ab1e...473599d. Read the comment docs.

aduth

The storePersist utility was created to be a generic solution for store persistence. The changes here introduce editor-specific awareness into rehydration logic, against this goal.

I could see this being:

An editor-specific middleware monitoring REDUX_REHYDRATE and overriding the isSidebarOpened property?
A condition within the reducer itself? Maybe too much environment awareness for a reducer.
Callback on the storePersist utility to manipulate the restored value before it is rehydrated?

jorgefilipecosta · 2017-11-03T17:33:57Z

Hi @aduth, thank you for your review and sharing the options. I end up refactoring the code to use a callback on storePersist.

aduth · 2017-11-06T17:20:53Z

editor/store-persist.js

 *
 * @return {Function}             Store enhancer
 */
-export default function storePersist( reducerKey, storageKey = DEFAULT_STORAGE_KEY, defaults = STORE_DEFAULTS ) {
+export default function storePersist( reducerKey, storageKey = DEFAULT_STORAGE_KEY, defaults = STORE_DEFAULTS, beforeReHydrate = identity ) {


Aside from making the argument signature a bit more awkward, it's not clear to me why we need defaults, and particularly why it needs to be hard-coded to editor-specific defaults. In the persistedState assignment below, wouldn't store.getState() already reflect these defaults?

Mentioned because we're importing STORE_DEFAULTS now in three places: the enhancer (which arguably should not be aware of these defaults itself), editor initialization, and the store reducer. It seems like only one of the last two should care about this. We already have the reducer using defaults (e.g. preferences), or Redux's createStore allows for a preloaded state as the second argument.

Also mentioning because as we add more arguments here, particularly when the arguments have defaults and the ordering isn't important, it seems like we might consider refactoring arguments to an object of options, e.g.

storePersist( { reducerKey: '...', storageKey: '...', beforeRehydrate: () => {}, } )

Also helps lend readability to the invocation, since without intimate familiarity with the utility, it is not clear what each of the arguments below is meant for:

storePersist( 'preferences', GUTENBERG_PREFERENCES_KEY, STORE_DEFAULTS, disableIsSidebarOpenedOnMobile ),

Hi @aduth, I also think using STORE_DEFAULTS in reducer is enough. And my tests seem to confirm that. So I applied the change and only the reducer sets the defaults. I also used an options object in storePersist. It is a work in progress I still need to update the tests if we pursue this solution.
In order for the enhancer to not include editor specific details, I added a parameter storePersistOptions and now is the provider when creating the store that passes details, still not the perfect solution but may be an improvement.

In order for the enhancer to not include editor specific details, I added a parameter storePersistOptions and now is the provider when creating the store that passes details, still not the perfect solution but may be an improvement.

As I see it, it's fine for details of editor/store.js to be editor-specific, particularly since it is responsible for bringing in and defining the editor's reducer/effects behaviors. The benefit of making the persistence enhancer editor-unaware is if we choose to make it available as a general-purpose utility, and arguably helps retain a nice separation of concerns. There isn't as obvious a benefit in separating the editor and its store initialization.

As I see it, it's fine for details of editor/store.js to be editor-specific, particularly since it is responsible for bringing in and defining the editor's reducer/effects behaviors.

Hi @aduth, I agree with you, with that in mind I think the current changes respect this. I moved editor specific defaults out of store persist and they are passed in editor/store.js.

As I see it, it's fine for details of editor/store.js to be editor-specific, particularly since it is responsible for bringing in and defining the editor's reducer/effects behaviors.

Hi @aduth, I agree with this, with that in mind I removed the editor specific details that were already in editor/store-persist.js and moved them into editor/store.js.

aduth · 2017-11-06T17:23:58Z

editor/store-persist.js

@@ -41,7 +47,7 @@ export default function storePersist( reducerKey, storageKey = DEFAULT_STORAGE_K

 			store.dispatch( {
 				type: 'REDUX_REHYDRATE',
-				payload: persistedState,
+				payload: beforeReHydrate( persistedState ),


Personal preference would be to capitalize as beforeRehydrate, since it doesn't seem that Re and Hydrate are separate words.

Corrected 👍

aduth · 2017-11-06T17:24:52Z

editor/utils/mobile.js

@@ -0,0 +1,14 @@
+
+const IS_MOBILE = window && window.innerWidth < 782;


I don't know that there's a great solution here, but the maintainability of this being kept in sync with variable.scss's $break-medium is concerning.

I noticed that we set the following class on PHP side:

if ( wp_is_mobile() ) { $admin_body_class .= ' mobile'; }

I was wondering if that could be useful in this context.

gziolo · 2017-11-08T09:55:43Z

It seems like you can pass the result of execution wp_is_mobile together with other settings when creating an editor instance and Redux state. Here is the place in the code where it could be included:

gutenberg/lib/client-assets.php

Line 763 in 959c2a2

$editor_settings = array(

. I have no idea if that is the correct approach in this context, but it would simplify things and ensure there is only one way of setting mobile view.

jorgefilipecosta · 2017-11-08T13:06:19Z

Hi @gziolo the problem with wp_is_mobile is that it is based on user agent and not on screen sizes. There are situations where even during the execution when a breakpoint changes (because the window resized) we want to execute some action.
To sync variables between sass and javascript I would suggest the usage of sass-variables-loader dev dependency.
I create a sample of it. Our sass variables are imported and exported in constants file so we guarantee that they are parsed just one time. In the constants file, we can also export variables that result from parsing sass variables.

gziolo · 2017-11-08T13:10:35Z

Similar issue applies to:

const IS_MOBILE = window && window.innerWidth < BREAK_MEDIUM;`

It's set once when the editor loads and won't change after you refresh the page. So it might not fully address the issue you are trying to solve.

jorgefilipecosta · 2017-11-08T13:28:15Z

It's set once when the editor loads and won't change after you refresh the page. So it might not fully address the issue you are trying to solve.

Yes in this case that was true because this function just needs to be applied on load when loading the prefs from localstorage. But I applied an improvement so we don't use the constant.
A sample case where we want dynamic actions on risizing is #3332. And we can mix this approaches and use this contants when setting responsive-redux breakpoints. Our sass variables become the unique place where we set window breakpoint and other places just use them.

aduth · 2017-11-09T20:03:46Z

editor/store-persist.js

- * @param {String}     reducerKey The reducer key to persist
- * @param {String}     storageKey The storage key to use
- * @param {Object}     defaults   Default values
+ * @param {String}     reducerKey          The reducer key to persist


I'm surprised this is not being flagged by the valid-jsdoc ESLint rule, since this function now has only a single parameter.

There are some ways to document expected object properties, outlined here:

http://usejsdoc.org/tags-type.html

aduth · 2017-11-09T20:04:55Z

editor/store.js

@@ -20,13 +22,19 @@ const GUTENBERG_PREFERENCES_KEY = `GUTENBERG_PREFERENCES_${ window.userSettings.

 /**
 * Creates a new instance of a Redux store.
+ * @param {Object}        storePersistOptions Additional store persist options


This function has no parameters.

aduth · 2017-11-09T20:07:25Z

editor/store.js

+		storePersist( {
+			reducerKey: 'preferences',
+			storageKey: GUTENBERG_PREFERENCES_KEY,
+			defaults: STORE_DEFAULTS,


If we're expecting the storePersist enhancer to handle persistence of a single reducer key, should we be passing the entire state object of defaults into it, or simply the default value of the persisted state subtree? This is made more complicated by the fact that we're assuming it'll be an object (nothing requiring it to be so, maybe it should at least be documented).

aduth · 2017-11-09T20:08:00Z

editor/utils/mobile.js

@@ -0,0 +1,13 @@
+import { BREAK_MEDIUM } from '../constants';


For consistency we should add the Internal dependencies DocBlock.

aduth · 2017-11-09T20:09:58Z

editor/utils/mobile.js

+ *
+ * @return {Object}            rehydrate payload with isSidebarOpened disabled if on mobile
+ */
+export const disableIsSidebarOpenedOnMobile = ( payload, isMobile = ( window && window.innerWidth < BREAK_MEDIUM ) ) => (


If window was not declared, I expect this would throw an error, same as:

foo && foo.bar // VM124:1 Uncaught ReferenceError: foo is not defined at <anonymous>:1:1

This is where an 'undefined' !== typeof window could be used. If you do this, I'd suggest not assigning this as the fallback parameter in arguments for readability's sake, and either moving it into a top-level variable, or performing the logic within the function itself.

aduth · 2017-11-09T20:11:30Z

editor/constants.js

@@ -0,0 +1,10 @@
+import scssVariables from '!!sass-variables-loader!./assets/stylesheets/_variables.scss';


Interesting loader. Why do you need the !! prefix?

Also, for consistency we should add the Internal dependencies DocBlock.

The !! disables other configured loaders during the import process. Without this flag or import was not being successful.

gziolo · 2017-11-10T08:14:31Z

editor/constants.js

@@ -0,0 +1,10 @@
+import scssVariables from '!!sass-variables-loader!./assets/stylesheets/_variables.scss';
+
+export { scssVariables };


We use only BREAK_MEDIUM in the code. Do we really need to expose all other variables? This list is quite huge, see: https://github.com/WordPress/gutenberg/blob/master/editor/assets/stylesheets/_variables.scss.

Regarding the other BREAK... constants we are creating there is a pull request where we add responsive-redux and I plan to use this variable as our breakpoints.
Regarding the export of scssVariables that's something we should discuss I think exposing it may be useful in the future there are situations where we may need to access a color or something similar. But we may decide that it is better to create constants for this cases like we are doing here to have a better control.

Regarding the export of scssVariables that's something we should discuss I think exposing it may be useful in the future there are situations where we may need to access a color or something similar.

It might, it might not :) My point of view here is as follows, it can be always added later when it's really needed. I try to follow YAGNI rule in such cases.

Yes that's true given that right now we don't have an external usage for scssVariables I followed your suggestion and removed the export :)

gziolo · 2017-11-10T08:43:22Z

editor/store-persist.js

@@ -41,7 +50,7 @@ export default function storePersist( reducerKey, storageKey = DEFAULT_STORAGE_K

 			store.dispatch( {
 				type: 'REDUX_REHYDRATE',
-				payload: persistedState,
+				payload: beforeRehydrate( persistedState ),


It looks a bit like a middleware inside another middleware :) I was wondering if that would be possible to create mobileMiddleware instead and intercept REDUX_REHYDRATE action and apply modification there. Something like this:

const mobile = store => next => action => { if ( action.type === 'REDUX_REHYDRATE' ) { return next( { type: 'REDUX_REHYDRATE', payload: disableIsSidebarOpenedOnMobile( action.payload ), } ); } return next( action ) }

It probably would have to be added in the chain of middlewares before this one. This way we would have the mobile logic hidden behind its own middleware. We could avoid using:

beforeRehydrate: disableIsSidebarOpenedOnMobile,

which is an elegant solution for this complex case, but at the same time difficult to follow.

jorgefilipecosta · 2017-11-10T15:19:00Z

I decided to apply the solution proposed by @gziolo and refactored the logic to be a middleware. Given that we found some possible store-persist improvements that now are not directly in the scope of this changes I separated these improvements into a different PR #3424.

gziolo · 2017-11-10T15:53:32Z

editor/utils/mobile.js

+ * Middleware
+ */
+
+export const mobileMiddleware = () => next => action => {


@aduth, @youknowriad did we agree on a standard place to put middlewares or is this PR still in progress?

That's a good question, I did not found a place for middlewares so I placed with the mobile logic. But if we define a place or there is a place already defined. I will correct it to be in the right place.

Given the precedents of with-change-detection and with-history—though not middlewares but reducer enhancers—it seems to me that, for the scope of this PR, editor/utils is the right place. My suggestion, however, for the sake of consistency, is to turn mobile.js into a proper mobile directory, including its test file and a README in the same mold as wCD and wH.

gziolo · 2017-11-10T15:56:51Z

editor/store.js

@@ -27,6 +28,7 @@ function createReduxStore() {
 	const enhancers = [
 		applyMiddleware( multi, refx( effects ) ),
 		storePersist( 'preferences', GUTENBERG_PREFERENCES_KEY ),
+		applyMiddleware( mobileMiddleware ),


I guess it's fine as it is, but I'm wondering why they do it differently in the official Redux docs: https://redux.js.org/docs/api/applyMiddleware.html#tips. In short, something like this:

let middleware = [ a, b ]; const store = createStore( reducer, preloadedState, applyMiddleware( ...middleware ) )

Redux documentation states that middleware should be applied before other enhancers, because enhancers may not see middleware changes otherwise. But this a specific case, we need this middleware to be applied after our storePersist enhancer because it changes storePersist. So I think because we need the middleware after the non-middleware enhancer we will not be able to use the same way used in redux docs.

I think I get it now. I wasn’t aware of the concept of enhancers. I found an example how the should be used and it matches what we have: https://redux.js.org/docs/api/compose.html

Thanks for spending your time explaing what’s happening here 🙇🏻

gziolo · 2017-11-10T15:58:52Z

I haven't tested yet, but I like it as it is. Thanks for addressing my feedback 😃 I will test it on Monday if it isn't merged by that time.

mcsf

This looks quite clear and works for me. The only thing I'm not confident assessing on my own is the pertinence of the loader for scssVariables, but Jorge's explanation seems good.

mcsf · 2017-11-12T10:35:00Z

editor/utils/test/mobile.js

+			dummy: 'non-dummy',
+		};
+		expect( disableIsSidebarOpenedOnMobile( input, false ) ).toEqual( output );
+	} );


Tiny suggestion: right now, the memory is fresh and the signature of disableIsSidebarOpenedOnMobile is known. Would it be clearer to add a variable like so:

const input = …, output = …, isMobile = true; expect( disableIsSidebarOpenedOnMobile( input, isMobile ) …

?

mcsf · 2017-11-12T10:39:49Z

editor/utils/mobile.js

+ * Middleware
+ */
+
+export const mobileMiddleware = () => next => action => {


Given the precedents of with-change-detection and with-history—though not middlewares but reducer enhancers—it seems to me that, for the scope of this PR, editor/utils is the right place. My suggestion, however, for the sake of consistency, is to turn mobile.js into a proper mobile directory, including its test file and a README in the same mold as wCD and wH.

jorgefilipecosta · 2017-11-13T15:22:47Z

Hi @mcsf thank you for your suggestions! They were applied.

gziolo · 2017-11-17T12:16:31Z

It was approved by @mcsf, please rebase and merge. There is no need to perform another review. Those was minor updates.

gziolo

I have already reviewed in the past and it still looks good 👍

aduth · 2017-11-17T22:49:27Z

package.json

@@ -87,6 +87,7 @@
    "react-markdown": "2.5.0",
    "react-test-renderer": "16.0.0",
    "sass-loader": "6.0.6",
+    "sass-variables-loader": "0.1.3",


We should have updated package-lock.json with this dependency addition. Users running npm 5+ will encounter changes introduced when installing from a fresh clone. The recommended Node version was bumped to the latest LTS in #3294 and will include npm 5+. I suggest installing this latest version.

Pushed to master in 1709207

Thank you for decting this problem and submiting a fast fix 👍 Unfortunately I missed the update and did not detected the probelm :(

jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label Nov 3, 2017

jorgefilipecosta self-assigned this Nov 3, 2017

jorgefilipecosta requested review from youknowriad, aduth, ellatrix, mcsf and gziolo November 3, 2017 15:52

aduth reviewed Nov 3, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch from 9f6a0da to dc3e650 Compare November 3, 2017 17:20

aduth reviewed Nov 6, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch from 62e7804 to fb8b957 Compare November 8, 2017 13:24

jorgefilipecosta force-pushed the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch 2 times, most recently from def4f83 to b5258fe Compare November 8, 2017 15:06

jorgefilipecosta mentioned this pull request Nov 9, 2017

Components: Show popovers fullscreen on mobile #3400

Merged

aduth reviewed Nov 9, 2017

View reviewed changes

gziolo reviewed Nov 10, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch 4 times, most recently from 3d58c84 to c4d994f Compare November 10, 2017 13:16

jorgefilipecosta mentioned this pull request Nov 10, 2017

Improved store persist. #3424

Merged

gziolo reviewed Nov 10, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch from c4d994f to 499ab1b Compare November 10, 2017 16:27

jorgefilipecosta mentioned this pull request Nov 10, 2017

Close sidebar when resizing from non mobile to mobile #3332

Merged

mcsf approved these changes Nov 12, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch from 499ab1b to 56d9bbe Compare November 13, 2017 15:14

gziolo approved these changes Nov 17, 2017

View reviewed changes

jorgefilipecosta added 2 commits November 17, 2017 13:05

Don't load isSidebarOpened preference from localStorage on mobile.

68f2a43

Added readme documentation to mobileMiddleware.

473599d

jorgefilipecosta force-pushed the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch from 56d9bbe to 473599d Compare November 17, 2017 13:05

jorgefilipecosta merged commit b87f59c into master Nov 17, 2017

jorgefilipecosta deleted the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch November 17, 2017 15:36

aduth reviewed Nov 17, 2017

View reviewed changes

		@@ -0,0 +1,14 @@

		const IS_MOBILE = window && window.innerWidth < 782;

		@@ -0,0 +1,13 @@
		import { BREAK_MEDIUM } from '../constants';

		@@ -0,0 +1,10 @@
		import scssVariables from '!!sass-variables-loader!./assets/stylesheets/_variables.scss';

		@@ -0,0 +1,10 @@
		import scssVariables from '!!sass-variables-loader!./assets/stylesheets/_variables.scss';

		export { scssVariables };

Don't load isSidebarOpened preference from localStorage on mobile. #3331

Don't load isSidebarOpened preference from localStorage on mobile. #3331

Conversation

jorgefilipecosta commented Nov 3, 2017 • edited Loading

Description

Testing

codecov bot commented Nov 3, 2017 • edited Loading

Codecov Report

aduth left a comment • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 3, 2017

Choose a reason for hiding this comment

jorgefilipecosta Nov 7, 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

gziolo Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

gziolo commented Nov 8, 2017

jorgefilipecosta commented Nov 8, 2017

gziolo commented Nov 8, 2017

jorgefilipecosta commented Nov 8, 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

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

jorgefilipecosta commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 10, 2017

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 13, 2017

gziolo commented Nov 17, 2017

gziolo 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

jorgefilipecosta commented Nov 3, 2017 •

edited

Loading

codecov bot commented Nov 3, 2017 •

edited

Loading

aduth left a comment •

edited

Loading

jorgefilipecosta Nov 7, 2017 •

edited

Loading

gziolo Nov 8, 2017 •

edited

Loading

gziolo Nov 10, 2017 •

edited

Loading