Edit Post: Refactor state handling for the sidebar #5435

gziolo · 2018-03-06T13:16:41Z

Description

Follow-up for #4777.

This PR aims to simplify the state management for the sidebars to make it easier to maintain with the upcoming changes from #5430. I figured out we can treat editor related sidebars in a similar way as plugins (maybe even combine them in the future) and store the active sidebar name together. I could take this approach because they are mutually exclusive in a sense that you can display only one of them at the same time in the same space.

At the same time, I refactored a few updated files to make them work with the new data module to get a better feeling how it all integrates with our codebase.

How Has This Been Tested?

Register sidebar using the following code:

wp.editPost.__experimentalRegisterSidebar( 
	"my-plugin/my-sidebar-x", 
	{
		title: "Name",
		render: () => { return wp.element.createElement( 'div', {}, 'My sidebar' ) }
	} 
);

To activate it use the following:

wp.editPost.__experimentalActivateSidebar( 
	"my-plugin/my-sidebar-x"
);

I tested the following manually:

Opened document settings using the cog icon in the header.
Closed document settings using the cog icon in the header.
Opened block settings using the block's dropdown menu and the Show Advanced Settings button.
Closed block settings using the block's dropdown menu and the Hide Advanced Settings button.
I could close the sidebar using the cross icon for both block and document tabs.
I could activate the plugin using the code shared above.
I could close the plugin sidebar using the cross icon.
Activated the plugin and refreshed the page and sidebar got deactivated, because the plugin is no longer registered.
Tested also a few flows when flipping the sidebar type using all navigation options.

All unit tests should still pass: npm test.

Screenshots (jpeg or gifs if applicable):

Types of changes

Refactoring.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

abotteram · 2018-03-06T20:01:53Z

edit-post/components/header/index.js

 		isPublishSidebarOpen: isPublishSidebarOpened( state ),
 		hasActiveMetaboxes: hasMetaBoxes( state ),
 		isSaving: isSavingMetaBoxes( state ),
 	} ),
 	{
-		onOpenGeneralSidebar: () => openGeneralSidebar( 'editor' ),
+		onOpenGeneralSidebar: openGeneralSidebar.bind( 'edit-post/document' ),


Should the argument be the second parameter?

Yeah, probably it's a better idea to stick with the arrow function 😃

gziolo · 2018-03-07T05:58:30Z

edit-post/api/sidebar.js

@@ -93,6 +96,5 @@ export function getSidebarSettings( name ) {
 * @return {void}
 */
 export function activateSidebar( name ) {


@omarreiss @xyfi @IreneStr, can we get rid of this function completely and start using dispatch( 'edit-post' ).openGeneralSidebar( name ); directly?

Question 2, should we rename openGeneralSidebar to openEditorSidebar and use the name editor sidebar in other places?

On the other hand, I created isEditorSidebarOpened selector to check if block or document settings are opened in the sidebar.

I guess it would be fine to get rid of this function. I still see the use for a higher level API like this because it simply requires less knowledge of the user. But maybe it is a bit premature to be adding API's like this one at this stage. I would like to make a knowledge matrix further down the road to map what knowledge is needed to solve a particular kind of problem in Gutenberg as a plugin author. Based on that we could always decide to reintroduce helpers like this one.

I will open a separate PR for that together with docs update. This PR got big once I started integrating data module. We can wrap it later back with something that you will find out helpful when integrating plugins.

jorgefilipecosta · 2018-03-07T14:57:48Z

edit-post/store/selectors.js

-	const generalSidebarOpen = getPreference( state, 'activeGeneralSidebar' ) !== null;
-	const publishSidebarOpen = state.publishSidebarActive;
-
-	return generalSidebarOpen || publishSidebarOpen;


We are still using a specific flag for publish sidebar, should we consolidate the behaviors and remove the flag from state?

Another point should we have a flag that tells if sidebars are open, and one property that tells the current sidebar that is open or will be open if sidebar flag is toggled?
The advantages being we can have a button to close and open the last sidebar, without resetting the last sidebar to close it. We would also be able to have open flags for different view sizes allowing us to recover the mobile behaviour where mobile sidebars open and close state is totally independent from desktop one.

We are still using a specific flag for publish sidebar, should we consolidate the behaviors and remove the flag from state?

That would be awesome. However, they have different UI. I didn't want to touch it as part of this PR, but rather focus on the code that might remove some code duplication between the editor and plugins.

Another point should we have a flag that tells if sidebars are open, and one property that tells the current sidebar that is open or will be open if sidebar flag is toggled?

This might be tricky because of how you interact with the editor. When you open the right side menu of the block you want to see the advanced setting of the block. When you click on the cog button in the header you rather expect to see the document settings, but definitely not the last open plugin. So I would say toggling for the last open sidebar might be not the best fit in many cases. Did you have something different in mind? I might not understand your reasoning correctly. This thing is really tricky because of so many factors involved :)

When you click on the cog button in the header you rather expect to see the document settings, but definitely not the last open plugin.

Yes, that's true, but in that case, the cog button whould trigger, an action to change the sidebar to the general one and another action to open the sidebar.

I think my basic idea is the following: we have one or more flags (e.g: mobile and desktop) that say if a sidebar is opened or not. So our sidebar checking logic is trivial.
We also have one field with the sidebar id of the sidebar that is opened or that if sidebars are shown should be rendered (with the open sidebar action) we can also trigger an action to change the sidebar.
The sidebar components would check the flag to see if sidebars are opened and the id of the sidebar to check if they should render or not.

This is something we can do after e.g: as part of a mobile improvement, or as part of a refactor of our own sidebars to use the extensibility mechanism.

Yes, we should definitely explore that further. Thanks for sharing your ideas 👍

jorgefilipecosta · 2018-03-09T15:09:28Z

edit-post/components/header/index.js

-	{ storeKey: 'edit-post' }
+export default compose(
+	withSelect( ( select ) => ( {
+		isEditorSidebarOpened: select( 'core/edit-post' ).isEditorSidebarOpened(),


Not directly related to this pr, and even if we decide to some change it should not be in this PR. We are always repeating select( 'core/edit-post' ). This makes me wonder if we should not do something like: const editPostSelect = select( 'core/edit-post' ). Or add an argument to withSelect like withSelect( 'core/edit-post', ( select )...

FWIW this is how I'd documented examples in the updated @wordpress/data documentation:

https://github.com/WordPress/gutenberg/tree/master/data#withselect-mapselecttoprops-function--function

While it means you can't do an implicit arrow return, I think the whole thing ends up being more readable overall (implicit arrow returns for objects are always questionable).

Actually, it has been discussed a few times in the past. I will find some link and share on Monday :) I might even share a similar idea you mentioned: withSelect( 'core/edit-post', ( select ). I think I saw @aduth using the following:

withSelect( ( select) => { const { mySelector } = select( 'core/edit-post' ); return { // some code here }; } );

@aduth I want to refactor all connect occurrences in the edit-post module next week to see if it has any impact on the module’s bundle size. I’ll follow your advice despite the fact that I personally like this short notation 😃

jorgefilipecosta · 2018-03-09T15:32:08Z

edit-post/store/selectors.js

+export function isEditorSidebarOpened( state ) {
+	const activeGeneralSidebar = getPreference( state, 'activeGeneralSidebar', null );
+
+	return [ 'edit-post/document', 'edit-post/block' ].includes( activeGeneralSidebar );


To check if a sidebar is active ( the general one) we check a property of the sidebar (which of tabs is opened). That seems like a good improvement to do next. Have an identifier for the general sidebar 'edit-post/general'. The logic to check if the general sidebar is opened or not should not use an internal property of the general sidebar which of the tabs is opened.
The tab of the general sidebar should only be used when rendering that sidebar, as it should be just a config of that sidebar. The tab logic of the general sidebar should be specified in an identical way to what a plugin needing tabs would do.

Array#includes is not polyfilled. This will error in IE11. Use _.includes instead.

Just wanted to clarify as I'm about to sign off for the weekend and apparently not thinking clear enough :)

Do you propose to combine those two sidebars into one that contains two tabs and display only one depending on the internal state (block vs document)?

I will add a commit in the PR #5528 that also solves this includes problem.

Thanks for fixing ❤️

jorgefilipecosta

I tested the changes here and I did not find any obvious regressions.
This simplifies a lot the code, nice job 👍
I think there are still some improvements/simplifications that could be made, I left some comments and discussions here can be a basis for future improvements. The improvements should not block the merging of this as the further improvements can be done in other PR's to avoid increasing the size of this PR.

aduth · 2018-03-09T16:52:40Z

edit-post/components/header/index.js

-	{ storeKey: 'edit-post' }
+export default compose(
+	withSelect( ( select ) => ( {
+		isEditorSidebarOpened: select( 'core/edit-post' ).isEditorSidebarOpened(),


FWIW this is how I'd documented examples in the updated @wordpress/data documentation:

https://github.com/WordPress/gutenberg/tree/master/data#withselect-mapselecttoprops-function--function

While it means you can't do an implicit arrow return, I think the whole thing ends up being more readable overall (implicit arrow returns for objects are always questionable).

aduth · 2018-03-09T17:00:37Z

edit-post/store/selectors.js

+export function isEditorSidebarOpened( state ) {
+	const activeGeneralSidebar = getPreference( state, 'activeGeneralSidebar', null );
+
+	return [ 'edit-post/document', 'edit-post/block' ].includes( activeGeneralSidebar );


Array#includes is not polyfilled. This will error in IE11. Use _.includes instead.

aduth · 2018-03-09T17:05:15Z

edit-post/store/selectors.js

+export function getActiveGeneralSidebarName( state ) {
+	const activeGeneralSidebar = getPreference( state, 'activeGeneralSidebar', null );
+
+	return activeGeneralSidebar && ( isEditorSidebarOpened( state ) || isPluginSidebarOpened( state ) ) ?


So... activeGeneralSidebar is not always expected to align with the value of isEditorSidebarOpened( state ) || isPluginSidebarOpened( state )? When does this occur? When someone tries to activate a sidebar for which there are no settings? Should that be allowed / optimized for?

I had an early return when there is no value set. It can be now removed as I created those two helper methods.
The things get complex with the plugins when you activate a plugin sidebar, leave the page and deactivate plugin. The value is persisted so next time you visit Gutenberg it tries to load the sidebar but there is no code there. That’s way isPluginSidebarOpen does additional checks. I hope to further optimize it once the new plugin API is finalized. We could also reset the value when about to render on initial page load. If you have better ideas let us know 😃

gziolo · 2018-03-09T17:20:43Z

@jorgefilipecosta thanks for your review and great comments. Let's continue the discussion on the next steps here in parallel to the ongoing work on #5430. As soon as that PR is merged we can take next steps 👍

aduth · 2018-03-09T17:22:00Z

Here's how the editor looks in IE11 after this merge:

See: #5435 (comment)

jorgefilipecosta · 2018-03-09T18:28:05Z

Thank you for catching this @aduth, a commit that solves this problem was added to PR #5528 that addresses a set of similar problems, and that PR was merged. I confirmed in IE11 that the problem is fixed.

gziolo · 2018-03-09T21:24:22Z

@aduth thanks for catching the issue with IE and includes. @jorgefilipecosta thanks for super quick fix. I searched for the occurrences of Array method and I found a few... I wasn’t aware they don’t work 😃

gziolo added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 6, 2018

gziolo self-assigned this Mar 6, 2018

gziolo requested a review from jorgefilipecosta March 6, 2018 13:16

gziolo added the [Status] In Progress Tracking issues with work in progress label Mar 6, 2018

abotteram reviewed Mar 6, 2018

View reviewed changes

Edit Post: Remove redundant activate sidebar action

06493ed

gziolo force-pushed the update/refactor-sidebar-state branch from 5aa31d4 to 24aed5b Compare March 7, 2018 05:41

gziolo commented Mar 7, 2018

View reviewed changes

gziolo force-pushed the update/refactor-sidebar-state branch from 24aed5b to b9f56de Compare March 7, 2018 07:56

Edit Post: Simplify sidebar handing in the store

bc8cced

gziolo force-pushed the update/refactor-sidebar-state branch from b9f56de to bc8cced Compare March 7, 2018 08:04

gziolo added 2 commits March 7, 2018 09:26

Edit Post: Improve Show Advanced Settings button

6f95ede

Edit Post: Refactor modified components to use new data module

4c9d517

gziolo force-pushed the update/refactor-sidebar-state branch from 4bfe64f to 4c9d517 Compare March 7, 2018 08:59

gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience and removed [Status] In Progress Tracking issues with work in progress labels Mar 7, 2018

gziolo requested a review from a team March 7, 2018 09:24

gziolo changed the title ~~[WIP] Edit Post: Refactor state handling for the sidebar~~ Edit Post: Refactor state handling for the sidebar Mar 7, 2018

jorgefilipecosta reviewed Mar 7, 2018

View reviewed changes

jorgefilipecosta reviewed Mar 9, 2018

View reviewed changes

jorgefilipecosta approved these changes Mar 9, 2018

View reviewed changes

aduth reviewed Mar 9, 2018

View reviewed changes

gziolo merged commit 2ee3351 into master Mar 9, 2018

gziolo deleted the update/refactor-sidebar-state branch March 9, 2018 17:19

jorgefilipecosta mentioned this pull request Mar 13, 2018

Restore sidebar state when resizing a window #5586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit Post: Refactor state handling for the sidebar #5435

Edit Post: Refactor state handling for the sidebar #5435

gziolo commented Mar 6, 2018 •

edited

Loading

abotteram Mar 6, 2018

gziolo Mar 7, 2018 •

edited

Loading

gziolo Mar 7, 2018

gziolo Mar 7, 2018 •

edited

Loading

omarreiss Mar 7, 2018

gziolo Mar 7, 2018

jorgefilipecosta Mar 7, 2018

gziolo Mar 9, 2018

jorgefilipecosta Mar 9, 2018 •

edited

Loading

gziolo Mar 9, 2018

jorgefilipecosta Mar 9, 2018

aduth Mar 9, 2018

gziolo Mar 9, 2018 •

edited

Loading

gziolo Mar 9, 2018 •

edited

Loading

jorgefilipecosta Mar 9, 2018 •

edited

Loading

aduth Mar 9, 2018

gziolo Mar 9, 2018

jorgefilipecosta Mar 9, 2018

gziolo Mar 9, 2018

jorgefilipecosta left a comment

aduth Mar 9, 2018

aduth Mar 9, 2018

aduth Mar 9, 2018

gziolo Mar 9, 2018

gziolo commented Mar 9, 2018 •

edited

Loading

aduth commented Mar 9, 2018

jorgefilipecosta commented Mar 9, 2018

gziolo commented Mar 9, 2018

Edit Post: Refactor state handling for the sidebar #5435

Edit Post: Refactor state handling for the sidebar #5435

Conversation

gziolo commented Mar 6, 2018 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Choose a reason for hiding this comment

gziolo Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Mar 7, 2018 • 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

jorgefilipecosta Mar 9, 2018 • 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

gziolo Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Mar 9, 2018 • 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

jorgefilipecosta 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

Choose a reason for hiding this comment

gziolo commented Mar 9, 2018 • edited Loading

aduth commented Mar 9, 2018

jorgefilipecosta commented Mar 9, 2018

gziolo commented Mar 9, 2018

gziolo commented Mar 6, 2018 •

edited

Loading

gziolo Mar 7, 2018 •

edited

Loading

gziolo Mar 7, 2018 •

edited

Loading

jorgefilipecosta Mar 9, 2018 •

edited

Loading

gziolo Mar 9, 2018 •

edited

Loading

gziolo Mar 9, 2018 •

edited

Loading

jorgefilipecosta Mar 9, 2018 •

edited

Loading

gziolo commented Mar 9, 2018 •

edited

Loading