Close sidebar when resizing from non mobile to mobile #3332

jorgefilipecosta · 2017-11-03T15:37:58Z

Description

This PR aims to close #1535. It adds a new feature that automatically closes the sidebar when resizing from non-mobile to mobile sizes.

Thanks to a suggestion by @youknowriad this PR now adds redux-responsive. It simplified the implementation of the feature.

How Has This Been Tested?

With sidebar open resize your window to a small size < 782. Verify that the sidebar automatically closes.
Open the sidebar see that you can open and close it in mobile sizes. Resize your window in small sizes with sidebar open and verify it does not close automatically. It only should close when going from big sizes > 782 to small sizes < 782.

Notes

The layout component was refactored to be class. I was asking my self if this logic should be on layout component or sidebar component. I decided on layout component to be more general and later we may want to add more resize actions that do not affect just the sidebar.

youknowriad

In the past, I used this lib https://github.com/AlecAivazis/redux-responsive to track browser dimensions.

Do you think, it could simplify the logic and just update the isEditorSidebarOpened reducer to return the default value based on the browser size if the reducer value is "null"?

Alternatively, we could extract this logic to its own component or HoC.

function Layout( { width } ) {
  console.log( width );
} 

export default withWindowSize( Layout )

jorgefilipecosta · 2017-11-03T18:37:25Z

Hi @youknowriad, I totally like the idea of using redux-responsive. It is one more dependency but in my option, it is worth it. And it will for sure be useful in other situations. I changed this PR to make usage of redux-responsive (and added it). Most of the code changes now are related with the refactor from function to class.

jasmussen · 2017-11-09T12:40:01Z

Nice work. Works nicely for me.

Can it also restore or reopen the sidebar if you had it open on desktop before? It doesn't have to if it adds complexity, but if it's trivial might be a nice enhancement. That would make it behave like this:

Start on desktop with sidebar open. Resize to small, sidebar closes, resize to large, sidebar opens again.
Start on desktop with sidebar closed. Resize to small, sidebar stays closed, resize to large, sidebar stays closed.

youknowriad · 2017-11-09T15:58:21Z

editor/layout/index.js

+class Layout extends Component {
+	componentWillReceiveProps( nextProps ) {
+		if ( nextProps.isSidebarOpened && nextProps.isMobile && ! this.props.isMobile ) {
+			this.props.toggleSidebar();


I wonder if we could achieve it with a Higher-order reducer instead.

const myReducer = combineReducer( { ... } ); const myEnhancedReducer = ( state, action ) => { const newState = myReducer( state, action ); if ( // check that isMobile and the previous state is not mobile using state and newSTate ) { newState.preferences = { ...newState.preferences, isSidebarOpened: false, }; } return newState; }

Do you think it's better this way? cc @aduth

Hi @youknowriad, for now, a Higher-order reduce could be an option. But when implementing the more advanced logic proposed by @jasmussen, I'm not certain if a high order reducer makes things simpler. Because if we want to automatically open the sidebar again when increasing the size if it was opened before we decreased size we need some state information which I was thinking on adding to the state of layout component and just dispatch close /open sidebar when needed. With a hight order component, I think we need to add more information to the redux state which is not bad I think but I'm not certain if this information necessary for this logic should be there.

I think if we want the proposed logic, we may need two reducer values. isSidebarOpened for desktop and isSidebarOpenedOnMobile which may simplify a lot of things :)

Yes, that is a very nice way of handling the situation!

Hi @youknowriad, to get a high order reducer here as it needs access to responsive and to preferences, it would need to be a top-level high order reducer, so it can access both responsive and preferences. It would be our first top-level high order reducer, is that right or I'm seeing something wrong?

True, but the more I think about it, the more two reducer values seems good. which also solves the persistence issue, since we'd have to persist only one of these two preferences.

Thank you for confirming my logic @youknowriad. Two reducer values sound like a good option for this case. But I think if we add an isSidebarOpenedMobile preference we still have the persistence issue. Because even the expected behavior of that ticket is for the sidebar to be closed on reload on mobile even if it was opened before. Our current store persists implementation stores all the preferences and we want it to be generic and not contain editor logic, so the best way to handle this problem I think is using the same logic we have now in PR #3331 but switch to ignore isSidebarOpenedMobile instead of isSidebarOpened.

Also with this change, we would at least need to change the isSidebarOppened selector to check if we should use the normal sidebarOppened flag or the mobile one. In the toggling places, we would also need some logic to decide if we are toggling mobile sidebar or the normal one. I think this are the changes needed but I may be pursuing a more complex path.

jasmussen · 2017-11-09T16:32:55Z

Just to clarify, you should only implement the logic of proposed if it doesn't add undue code complexity. I'm describing a rare edge case that's non destructive, so if the code is much cleaner and better without that logic I'd definitely defer to Riad.

jorgefilipecosta · 2017-11-09T16:37:14Z

Hi @jasmussen, I understood, I just specifying a case where if we have the current implementation handling it may be simpler than in a hight order reducer. For us to decide on the best approach to use.

gziolo · 2017-11-10T20:50:26Z

Let me propose an alternative solution. It probably have some drawbacks in terms of UI and code duplication, but it might solved all issues discussed so far. I was trying to find an implementation which would avoid using tweaking Redux stare for mobile. It turns out that we already use a pattern where we display controls for the block in a different place for mobile devices and the rest (we use 2 different components that which are displayed in turns).

I thought why not take a similar approach with sidebar. If we would display the sidebar only for non-mobile users we could keep the current logic untouched. The thing is that we would have to provide a parallel UI for mobile view. It seems like an overlay would fit the best to what we want to have here. Such approach will keep mobile completely independent from the toggle state of the sidebar. I’m not sure if we would be happy to use overlay here, but honestly speaking what we have now looks this way anyway. The best part of having the overlay is that you don’t need to persist its state as users expect to have it closed by default 😃

jorgefilipecosta · 2017-11-13T11:33:44Z

Hi @gziolo, we would need some state information to know if we should show the sidebar overlay or not. This flag indicating if we show sidebar overlay is essentially a showSidebarMobile so I think your suggestion is similar to @youknowriad idea.
If this flag is saved in the redux state we need a store middleware to not persist it. If this flag is saved in the component state we don't need it, but it may be strange to save the normal sidebar flag in redux and not save the sidebar overlay flag.

gziolo · 2017-11-17T08:49:55Z

editor/reducer.js

@@ -657,6 +658,13 @@ export function metaBoxes( state = defaultMetaBoxState, action ) {
 	}
 }

+// Create responsive reducer with lib default breakpoints excluding small where we are currently using 782.
+const responsive = createResponsiveStateReducer( {


Should we use variables imported from .scss file instead?

Yes, that's the plan, but the PR that adds an scss importer #3331 is not yet merged. It would be perfect if we can a new review on it and land it before this PR so we can make use of the importer :)

You referenced wrong PR number :)

Yes, you are right, the correct PR number is #3331. (comment was updated)

gziolo · 2017-11-17T08:56:37Z

Hi @gziolo, we would need some state information to know if we should show the sidebar overlay or not. This flag indicating if we show sidebar overlay is essentially a showSidebarMobile so I think your suggestion is similar to @youknowriad idea.
If this flag is saved in the redux state we need a store middleware to not persist it. If this flag is saved in the component state we don't need it, but it may be strange to save the normal sidebar flag in redux and not save the sidebar overlay flag.

Yes, it's a similar idea. We should have two different ways of controlling sidebar on mobile and desktop. Pick whatever you prefer in this case. I guess what @youknowriad proposed is easier to implement because it doesn't require changes in the UI thus moving additional complexity there. I think this is what @jasmussen wanted to avoid here. Saying that and reading:

so the best way to handle this problem I think is using the same logic we have now in PR #3331 but switch to ignore isSidebarOpenedMobile instead of isSidebarOpened.

I think that this proposal is a reasonable way of tackling this complex use case :)

codecov · 2017-11-20T15:00:14Z

Codecov Report

Merging #3332 into master will increase coverage by 0.03%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #3332      +/-   ##
==========================================
+ Coverage   37.64%   37.67%   +0.03%     
==========================================
  Files         279      279              
  Lines        6732     6736       +4     
  Branches     1225     1226       +1     
==========================================
+ Hits         2534     2538       +4     
  Misses       3537     3537              
  Partials      661      661

Impacted Files	Coverage Δ
editor/actions.js	`47.16% <ø> (ø)`	⬆️
editor/store-defaults.js	`100% <ø> (ø)`	⬆️
editor/store.js	`83.33% <ø> (ø)`	⬆️
editor/selectors.js	`93.46% <100%> (+0.03%)`	⬆️
editor/reducer.js	`90.37% <100%> (+0.1%)`	⬆️
editor/utils/mobile/index.js	`75% <80%> (+17.85%)`	⬆️
utils/viewport.js	`0% <0%> (-100%)`	⬇️

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 467f044...6dfdfe9. Read the comment docs.

jorgefilipecosta · 2017-11-20T15:15:02Z

Hi @gziolo, @youknowriad, I updated this PR to implement the two flags approach, creating the behavior described by @jasmussen. Feel free to have a new look.

gziolo · 2017-11-24T10:54:36Z

editor/components/block-settings-menu/block-inspector-button.js

@@ -46,13 +46,21 @@ export function BlockInspectorButton( {
 export default connect(
 	( state ) => ( {
 		isSidebarOpened: isEditorSidebarOpened( state ),
+		isMobile: ! state.responsive.greaterThan.medium,


It would be nice to hide this logic behind isMobile( state ) selector. I think this is a general pattern to always access state through selectors. @youknowriad can you confirm?

I added an isMobile selector, it simplifies the logic and is more in line with the rest of our code base :)

gziolo · 2017-11-24T10:56:25Z

editor/reducer.js

@@ -667,6 +679,15 @@ export function metaBoxes( state = defaultMetaBoxState, action ) {
 	}
 }

+// Create responsive reducer with lib default breakpoints excluding small where we are currently using 782.


I don't understand the part about 782, can you explain?

I corrected the comment, the comment was valid before we had scss imports (added in parallel in other PR) and the values were hardcoded. But now it is not valid.

gziolo · 2017-11-24T11:13:03Z

editor/sidebar/header.js

@@ -48,9 +49,16 @@ export default connect(
 	( state ) => ( {
 		panel: getActivePanel( state ),
 		count: getSelectedBlockCount( state ),
+		isMobile: ! state.responsive.greaterThan.medium,


We have 3 places where we need to pull isMobile value. Every time this value changes, React will detect that props have changed so it will try to re-render component even though onToggleSidebar method is not dynamically created. I'm wondering if this component will re-render when isMobile value changes with all those optimizations applied. It's a very interesting use case :)

gziolo · 2017-11-24T11:13:16Z

editor/store-defaults.js

 export const PREFERENCES_DEFAULTS = {
 	mode: 'visual',
-	isSidebarOpened: ! viewPort.isExtraSmall(),
+	isSidebarOpened: true,


gziolo · 2017-11-24T11:14:18Z

editor/utils/mobile/index.js

-export const disableIsSidebarOpenedOnMobile = ( payload, isMobile = isMobileChecker() ) => (
-	isMobile ? { ...payload, isSidebarOpened: false } : payload
+export const disableIsSidebarOpenedOnMobile = ( payload ) => (
+	payload.isSidebarOpenedMobile ? { ...payload, isSidebarOpenedMobile: false } : payload


gziolo · 2017-11-24T11:21:47Z

This looks very good, nice refactoring was done together with the behavior implemented 👍

One more thing to consider. Would it make sense we move this whole isMobile detection to the middleware?

export const mobileMiddleware = ( { getState } ) => next => action => {
	if ( action.type === 'REDUX_REHYDRATE' ) {
		// ...
	}
	if ( action.type === 'TOGGLE_SIDEBAR' ) {
		return next( {
			type: 'TOGGLE_SIDEBAR',
			isMobile: isMobile( getState() ),
		} );
	}
	return next( action );
};

Alternative solution with the updated action name:

if ( action.type === 'TOGGLE_SIDEBAR' && isMobile( getState() ) ) {
	return next( {
		type: 'TOGGLE_SIDEBAR_MOBILE',
	} );
}

jorgefilipecosta · 2017-11-24T19:44:31Z

Hi @gziolo, unfortunately, It did not occur to me using the mobile middleware to add the required flag but doing it simplifies a lot the logic. So I improved this PR with your suggestion. Thank you!

gziolo · 2017-11-29T10:57:31Z

editor/utils/mobile/index.js

 	if ( action.type === 'REDUX_REHYDRATE' ) {
 		return next( {
 			type: 'REDUX_REHYDRATE',
 			payload: disableIsSidebarOpenedOnMobile( action.payload ),
 		} );
 	}
+	if ( action.type === 'TOGGLE_SIDEBAR' ) {
+		return next( {


You can code it using toggleSidebar( isMobile( getState() ) to take advantage of the existing toggleSidebar action creator.

gziolo · 2017-11-29T10:59:56Z

editor/utils/mobile/index.js

- */
-const isMobileChecker = () => 'object' === typeof window && window.innerWidth < BREAK_MEDIUM;
-
+import { isMobile } from '../../selectors';


Missing docblock for Internal dependencies

gziolo · 2017-11-29T11:06:08Z

editor/reducer.js

@@ -472,7 +481,10 @@ export function blockInsertionPoint( state = {}, action ) {
 export function preferences( state = PREFERENCES_DEFAULTS, action ) {
 	switch ( action.type ) {
 		case 'TOGGLE_SIDEBAR':
-			return {
+			return action.isMobile ? {


For my own curiosity, I tried a different syntax to avoid 2 objects :)

const isSidebarOpenedKey = action.isMobile ? 'isSidebarOpenedMobile' : 'isSidebarOpened'; return { ...state, [ isSidebarOpenedKey ]: ! state[ isSidebarOpenedKey ], };

It might be a bit harder to read, but sharing with you anyway :)

ES.next is awesome ❤️

gziolo · 2017-11-29T11:08:13Z

It looks great after all those iterations, thanks for working on it. I will test later today before I accept.

gziolo · 2017-11-29T17:11:29Z

There is still one thing I'm not sure about. Steps to reproduce:

open in mobile view
open sidebar
resize to desktop
sidebar disappears
resize to mobile again
sidebar takes full screen

gziolo

Code looks good, there is one use case which is opened for discussion, but I wouldn't treat it as a blocker. Just something to keep in mind. Otherwise, it all works as expected!

jorgefilipecosta · 2017-11-30T17:06:17Z

All the changes were applied @gziolo thank you for your efforts in reviewing.
I tried to replicate the behavior you described but I'm only able to replicate it if the sidebar is closed on the desktop. So when I open sidebar on mobile and than I resize to the desktop the sidebar closes than if I resize to mobile it opens again. I think this is the expected behavior for now but in the future, we can, of course, try to improve some aspect :)

…when resizing to small sizes.

…d old mobile logic; Updated disableIsSidebarOpenedOnMobile function part of mobile middleware to use the new isSidebarOpenedMobile flag;

…arOpened selector.

…le flag.

…mobile flag in TOGGLE_SIDEBAR actions.

jorgefilipecosta force-pushed the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch from e88b936 to 51328dc Compare November 3, 2017 15:41

jorgefilipecosta self-assigned this Nov 3, 2017

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

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

jorgefilipecosta changed the title ~~Close sidebar on resizing from non mobile to mobile~~ Close sidebar when resizing from non mobile to mobile Nov 3, 2017

youknowriad reviewed Nov 3, 2017

View reviewed changes

This was referenced Nov 8, 2017

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

Merged

Components: Show popovers fullscreen on mobile #3400

Merged

jorgefilipecosta requested a review from jasmussen November 9, 2017 12:18

youknowriad reviewed Nov 9, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch from 697cb36 to 21acd2c Compare November 13, 2017 11:28

gziolo reviewed Nov 17, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch 4 times, most recently from dd9ea1c to f2c446b Compare November 20, 2017 14:50

gziolo reviewed Nov 24, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch 2 times, most recently from e8788e9 to ede88b0 Compare November 24, 2017 19:39

jorgefilipecosta force-pushed the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch 3 times, most recently from c7d1f35 to 6084c4c Compare November 24, 2017 22:41

gziolo reviewed Nov 29, 2017

View reviewed changes

gziolo approved these changes Nov 29, 2017

View reviewed changes

jorgefilipecosta force-pushed the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch from 6084c4c to 7fdc9c2 Compare November 30, 2017 16:45

jorgefilipecosta added 5 commits December 1, 2017 10:23

Added redux-resposive usage to implement automatic sidebar disabling …

c5199c9

…when resizing to small sizes.

Updated store-defaults with the isSidebarOpenedMobile flag and remove…

f3427e2

…d old mobile logic; Updated disableIsSidebarOpenedOnMobile function part of mobile middleware to use the new isSidebarOpenedMobile flag;

Implemented isMobile selector and added mobile logic to isEditorSideb…

763dccc

…arOpened selector.

Updated toggleSidebar action and associated reducer with a new isMobi…

cc155e1

…le flag.

Added functionality in mobile middleware to automatically add the is …

6dfdfe9

…mobile flag in TOGGLE_SIDEBAR actions.

jorgefilipecosta force-pushed the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch from 7fdc9c2 to 6dfdfe9 Compare December 1, 2017 10:23

jorgefilipecosta merged commit 97a1d4c into master Dec 1, 2017

jorgefilipecosta deleted the fix/close-sidebar-on-resizing-from-non-mobile-to-mobile branch December 1, 2017 10:34

snyk-bot mentioned this pull request Sep 21, 2022

[Snyk] Security upgrade lerna from 3.22.1 to 5.5.2 MaxMood96/gutenberg#75

Open

Close sidebar when resizing from non mobile to mobile #3332

Close sidebar when resizing from non mobile to mobile #3332

Conversation

jorgefilipecosta commented Nov 3, 2017 • edited Loading

Description

How Has This Been Tested?

Notes

youknowriad left a comment • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 3, 2017

jasmussen commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Nov 9, 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

jasmussen commented Nov 9, 2017

jorgefilipecosta commented Nov 9, 2017

gziolo commented Nov 10, 2017 • edited Loading

jorgefilipecosta commented Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

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

codecov bot commented Nov 20, 2017 • edited Loading

Codecov Report

jorgefilipecosta commented Nov 20, 2017

Choose a reason for hiding this comment

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

gziolo commented Nov 24, 2017 • edited Loading

jorgefilipecosta commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

gziolo commented Nov 29, 2017

gziolo commented Nov 29, 2017 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 30, 2017 • edited Loading

jorgefilipecosta commented Nov 3, 2017 •

edited

Loading

youknowriad left a comment •

edited

Loading

youknowriad Nov 9, 2017 •

edited

Loading

gziolo commented Nov 10, 2017 •

edited

Loading

jorgefilipecosta commented Nov 13, 2017 •

edited

Loading

jorgefilipecosta Nov 17, 2017 •

edited

Loading

codecov bot commented Nov 20, 2017 •

edited

Loading

jorgefilipecosta Nov 24, 2017 •

edited

Loading

gziolo commented Nov 24, 2017 •

edited

Loading

gziolo Nov 29, 2017 •

edited

Loading

gziolo commented Nov 29, 2017 •

edited

Loading

jorgefilipecosta commented Nov 30, 2017 •

edited

Loading