Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Edit Site: Remove templateIds state #22893

Merged
merged 17 commits into from
Jul 6, 2020
Merged

Edit Site: Remove templateIds state #22893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e8b0235
Edit Site: Remove templateIds state and selector
ockham Jun 4, 2020
f33b393
Remove some server-side templateIds logic
ockham Jun 4, 2020
5dee808
Remove now-obsolete REMOVE_TEMPLATE action
ockham Jun 4, 2020
77f2fbc
Replace ADD_TEMPLATE with SET_TEMPLATE
ockham Jun 4, 2020
6b53c4b
Fix unit test
ockham Jun 5, 2020
0722365
Remove templatePartIds state and selector
ockham Jun 22, 2020
14b1cca
Update comment
ockham Jun 23, 2020
2f7100f
Revert "Fix unit test"
ockham Jun 23, 2020
ad38258
Remove templateType from editSiteInitialState
ockham Jun 23, 2020
9cc4e27
Update comment
ockham Jun 23, 2020
adf85bd
Fix comment :sweat_smile:
ockham Jun 23, 2020
be0bbbe
Revert "Revert "Fix unit test""
ockham Jun 29, 2020
6e580b6
Update JSDoc
ockham Jun 29, 2020
8568997
Add FIXME to test
ockham Jul 6, 2020
894d7e0
Add waitForSelector for template switcher
ockham Jul 6, 2020
7642660
Add back templateType to editSiteInitialState
ockham Jul 6, 2020
432dc97
Revert "Add waitForSelector for template switcher"
ockham Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 3 additions & 22 deletions lib/edit-site-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,30 +145,11 @@ function gutenberg_edit_site_init( $hook ) {
}
$settings['styles'] = gutenberg_get_editor_styles();

$template_ids = array();
$template_part_ids = array();
foreach ( get_template_types() as $template_type ) {
noahtallen marked this conversation as resolved.
Show resolved Hide resolved
// Skip 'embed' for now because it is not a regular template type.
// Skip 'index' because it's a fallback that we handle differently.
if ( in_array( $template_type, array( 'embed', 'index' ), true ) ) {
continue;
}

$current_template = gutenberg_find_template_post_and_parts( $template_type );
if ( isset( $current_template ) ) {
$template_ids[ $template_type ] = $current_template['template_post']->ID;
$template_part_ids = $template_part_ids + $current_template['template_part_ids'];
}
}

$settings['editSiteInitialState'] = array();

$settings['editSiteInitialState']['templateType'] = 'wp_template';
$settings['editSiteInitialState']['templateIds'] = array_values( $template_ids );
$settings['editSiteInitialState']['templatePartIds'] = array_values( $template_part_ids );

$settings['editSiteInitialState']['showOnFront'] = get_option( 'show_on_front' );
$settings['editSiteInitialState']['page'] = array(
$settings['editSiteInitialState']['templateType'] = 'wp_template';
ockham marked this conversation as resolved.
Show resolved Hide resolved
$settings['editSiteInitialState']['showOnFront'] = get_option( 'show_on_front' );
ockham marked this conversation as resolved.
Show resolved Hide resolved
$settings['editSiteInitialState']['page'] = array(
ockham marked this conversation as resolved.
Show resolved Hide resolved
'path' => '/',
'context' => 'page' === $settings['editSiteInitialState']['showOnFront'] ? array(
'postType' => 'page',
Expand Down
2 changes: 1 addition & 1 deletion lib/template-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ function gutenberg_template_loader_filter_block_editor_settings( $settings ) {
}

// If this is the Site Editor, auto-drafts for template parts have already been generated
// through `gutenberg_find_template_post_and_parts` in `gutenberg_edit_site_init`.
// through `filter_rest_wp_template_part_query`, when called via the REST API.
if ( isset( $settings['editSiteInitialState'] ) ) {
return $settings;
}
Expand Down
16 changes: 6 additions & 10 deletions packages/edit-site/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ export function setTemplate( templateId ) {
}

/**
* Returns an action object used to add a template.
* Adds a new template, and sets it as the current template.
*
* @param {Object} template The template.
*
* @return {Object} Action object.
* @return {Object} Action object used to set the current template.
*/
export function* addTemplate( template ) {
const newTemplate = yield dispatch(
Expand All @@ -66,32 +66,28 @@ export function* addTemplate( template ) {
template
);
return {
type: 'ADD_TEMPLATE',
type: 'SET_TEMPLATE',
templateId: newTemplate.id,
};
}

/**
* Returns an action object used to remove a template.
* Removes a template, and updates the current page and template.
*
* @param {number} templateId The template ID.
*
* @return {Object} Action object.
* @return {Object} Action object used to set the current page and template.
*/
export function* removeTemplate( templateId ) {
yield apiFetch( {
path: `/wp/v2/templates/${ templateId }`,
method: 'DELETE',
} );
yield dispatch(
return dispatch(
'core/edit-site',
'setPage',
yield select( 'core/edit-site', 'getPage' )
);
return {
type: 'REMOVE_TEMPLATE',
templateId,
};
}

/**
Expand Down
34 changes: 0 additions & 34 deletions packages/edit-site/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ export function homeTemplateId( state ) {
export function templateId( state, action ) {
switch ( action.type ) {
case 'SET_TEMPLATE':
case 'ADD_TEMPLATE':
noahtallen marked this conversation as resolved.
Show resolved Hide resolved
case 'SET_PAGE':
return action.templateId;
}
Expand Down Expand Up @@ -143,7 +142,6 @@ export function templatePartId( state, action ) {
export function templateType( state, action ) {
switch ( action.type ) {
case 'SET_TEMPLATE':
case 'ADD_TEMPLATE':
case 'SET_PAGE':
return 'wp_template';
case 'SET_TEMPLATE_PART':
Expand All @@ -153,36 +151,6 @@ export function templateType( state, action ) {
return state;
}

/**
* Reducer returning the list of template IDs.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function templateIds( state = [], action ) {
switch ( action.type ) {
case 'ADD_TEMPLATE':
return [ ...state, action.templateId ];
case 'REMOVE_TEMPLATE':
return state.filter( ( id ) => id !== action.templateId );
}

return state;
}

/**
* Reducer returning the list of template part IDs.
*
* @param {Object} state Current state.
*
* @return {Object} Updated state.
*/
export function templatePartIds( state = [] ) {
return state;
}

/**
* Reducer returning the page being edited.
*
Expand Down Expand Up @@ -219,8 +187,6 @@ export default combineReducers( {
templateId,
templatePartId,
templateType,
templateIds,
templatePartIds,
page,
showOnFront,
} );
22 changes: 0 additions & 22 deletions packages/edit-site/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,28 +123,6 @@ export function getTemplateType( state ) {
return state.templateType;
}

/**
* Returns the current template IDs.
*
* @param {Object} state Global application state.
*
* @return {number[]} Template IDs.
*/
export function getTemplateIds( state ) {
return state.templateIds;
}

/**
* Returns the current template part IDs.
*
* @param {Object} state Global application state.
*
* @return {number[]} Template part IDs.
*/
export function getTemplatePartIds( state ) {
return state.templatePartIds;
}

/**
* Returns the current page object.
*
Expand Down
14 changes: 4 additions & 10 deletions packages/edit-site/src/store/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe( 'actions', () => {
} );

describe( 'addTemplate', () => {
it( 'should yield the DISPATCH control to create the template and return the ADD_TEMPLATE action', () => {
it( 'should yield the DISPATCH control to create the template and return the SET_TEMPLATE action', () => {
const template = { slug: 'index' };
const newTemplate = { id: 1 };

Expand All @@ -45,7 +45,7 @@ describe( 'actions', () => {
} );
expect( it.next( newTemplate ) ).toEqual( {
value: {
type: 'ADD_TEMPLATE',
type: 'SET_TEMPLATE',
templateId: newTemplate.id,
},
done: true,
Expand All @@ -54,7 +54,7 @@ describe( 'actions', () => {
} );

describe( 'removeTemplate', () => {
it( 'should yield the API_FETCH control, yield the SELECT control to set the page by yielding the DISPATCH control for the SET_PAGE action, and return the REMOVE_TEMPLATE action', () => {
it( 'should yield the API_FETCH control and yield the SELECT control to set the page by yielding the DISPATCH control for the SET_PAGE action', () => {
const templateId = 1;
const page = { path: '/' };

Expand All @@ -72,19 +72,13 @@ describe( 'actions', () => {
selectorName: 'getPage',
args: [],
} );
// @FIXME: Test for done: true.
expect( it.next( page ).value ).toEqual( {
type: 'DISPATCH',
storeKey: 'core/edit-site',
actionName: 'setPage',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to check for done: true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had it check for done: true but removed it after unit tests were failing, and I couldn't quite work out why 😬

I'll try reverting that commit 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing again. I don't quite see why this isn't working, do you have any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I un-reverted that commit. AFAICS, this is failing since we're returning a dispatch action here, which is a generator itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dispatch just returns an object. Something else is wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master: 274 queries, 148,8 ms
This branch: 28 queries, 5.4 ms

🎤 💧

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pretty nice performance improvement <3

Copy link
Contributor

@jeyip jeyip Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but could you elaborate on how you were able to access those perf numbers, Bernie?

I'm trying to access the debug bar locally but can't seem to get the debug dashboard to display. (I'm using query params like so http://localhost:8888/wp-admin/admin.php?page=gutenberg-edit-site&debug-bar=1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need the debug bar plugin installed and a few config constants set. You can use a .wp-env.override.json in your local Gutenberg source checkout dir to achieve both. This is mine (also links theme-experiments themes).

{
        "core": "../wordpress-develop/build",
        "plugins": [ ".", "https://downloads.wordpress.org/plugin/debug-bar.zip" ],
        "themes": [ "WordPress/theme-experiments" ],
        "config": { "WP_DEBUG": true, "SCRIPT_DEBUG": true, "SAVEQUERIES": true }
}

args: [ page ],
} );
expect( it.next() ).toEqual( {
value: {
type: 'REMOVE_TEMPLATE',
templateId,
},
done: true,
} );
} );
} );

Expand Down
65 changes: 0 additions & 65 deletions packages/edit-site/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import {
templateId,
templatePartId,
templateType,
templateIds,
templatePartIds,
page,
showOnFront,
} from '../reducer';
Expand Down Expand Up @@ -100,15 +98,6 @@ describe( 'state', () => {
).toEqual( 2 );
} );

it( 'should update when a template is added', () => {
expect(
templateId( 1, {
type: 'ADD_TEMPLATE',
templateId: 2,
} )
).toEqual( 2 );
} );

it( 'should update when a page is set', () => {
expect(
templateId( 1, {
Expand Down Expand Up @@ -157,14 +146,6 @@ describe( 'state', () => {
).toEqual( 'wp_template' );
} );

it( 'should update when a template is added', () => {
expect(
templateType( undefined, {
type: 'ADD_TEMPLATE',
} )
).toEqual( 'wp_template' );
} );

it( 'should update when a page is set', () => {
expect(
templateType( undefined, {
Expand All @@ -182,52 +163,6 @@ describe( 'state', () => {
} );
} );

describe( 'templateIds()', () => {
it( 'should apply default state', () => {
expect( templateIds( undefined, {} ) ).toEqual( [] );
} );

it( 'should default to returning the same state', () => {
const state = {};
expect( templateIds( state, {} ) ).toBe( state );
} );

it( 'should add template IDs', () => {
expect(
templateIds( deepFreeze( [ 1 ] ), {
type: 'ADD_TEMPLATE',
templateId: 2,
} )
).toEqual( [ 1, 2 ] );
} );

it( 'should remove template IDs', () => {
expect(
templateIds( deepFreeze( [ 1, 2 ] ), {
type: 'REMOVE_TEMPLATE',
templateId: 2,
} )
).toEqual( [ 1 ] );
expect(
templateIds( deepFreeze( [ 1, 2 ] ), {
type: 'REMOVE_TEMPLATE',
templateId: 1,
} )
).toEqual( [ 2 ] );
} );
} );

describe( 'templatePartIds()', () => {
it( 'should apply default state', () => {
expect( templatePartIds( undefined, {} ) ).toEqual( [] );
} );

it( 'should default to returning the same state', () => {
const state = {};
expect( templatePartIds( state, {} ) ).toBe( state );
} );
} );

describe( 'page()', () => {
it( 'should apply default state', () => {
expect( page( undefined, {} ) ).toEqual( {} );
Expand Down
16 changes: 0 additions & 16 deletions packages/edit-site/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import {
getTemplateId,
getTemplatePartId,
getTemplateType,
getTemplateIds,
getTemplatePartIds,
getPage,
getShowOnFront,
} from '../selectors';
Expand Down Expand Up @@ -131,20 +129,6 @@ describe( 'selectors', () => {
} );
} );

describe( 'getTemplateIds', () => {
it( 'returns the template IDs', () => {
const state = { templateIds: {} };
expect( getTemplateIds( state ) ).toBe( state.templateIds );
} );
} );

describe( 'getTemplatePartIds', () => {
it( 'returns the template part IDs', () => {
const state = { templatePartIds: {} };
expect( getTemplatePartIds( state ) ).toBe( state.templatePartIds );
} );
} );

describe( 'getPage', () => {
it( 'returns the page object', () => {
const state = { page: {} };
Expand Down