Add ability to set page/post template #4662

jorgefilipecosta · 2018-01-24T17:21:47Z

Description

The main objective of this PR is to add the ability to change page/post template.
Fixes: #991

How Has This Been Tested?

Verify your theme has custom page templates. If not create a new file in the theme folder with a simple template: <?php /* Template Name: Test templage Template Post Type: post, page, event */ ?> test. (Change the theme and recharge to it if the new template is not available.
Set a page template. Save the post verify the page template gets used.
Do the same process for a "post" post type, verify the panel is name "Post Attributes" instead of "Page Attributes".
Verify the Post Attributes meta box is not appearing in advanced post settings.

Types of changes

Used correct label, for Post, attributes from the post types label.
Removed Post Attributes default core meta box.
Corrected "Page Attributes" panel to show if post type supports page attributes or if the there are custom templates available (same logic as in core).

youknowriad · 2018-01-24T17:38:54Z

editor/components/page-attributes/order.js

+import { getCurrentPostType, getEditedPostAttribute } from '../../store/selectors';
+
+export function PageAttributesOrder( { onUpdateOrder, instanceId, order, postType } ) {
+	if ( ! get( postType, 'data.supports.page-attributes', false ) ) {


We have a component for these checks PostTypeSupportCheck, can we reuse it here?

Hi @youknowriad, PostTypeSupportCheck was updated in this PR to use the same condition used in core to decide when to show Post Attributes metabox: ( ! supportsPageAttributes && isEmpty( availableTemplates ).

In this case, we really want check if post type supports page attributes because it is the condition also used in the core to check when to show Order input field.

youknowriad · 2018-01-24T17:40:06Z

editor/components/page-attributes/template.js

+
+	/* eslint-disable jsx-a11y/no-onchange */
+	return (
+		<div className="editor-page-template">


Should this be editor-page-attributes__template to match our CSS guidelines?

aduth · 2018-01-24T18:22:16Z

editor/components/page-attributes/template.js

+	}
+	const selectId = `template-selector-${ instanceId }`;
+
+	// Disable reason: A select with an onchange throws a warning


This is a statement of fact, not a reason for disabling.

aduth · 2018-01-24T18:22:27Z

editor/components/page-attributes/template.js

+
+	// Disable reason: A select with an onchange throws a warning
+
+	/* eslint-disable jsx-a11y/no-onchange */


We are never re-enabling the ESLint rule.

aduth · 2018-01-24T18:22:41Z

editor/components/page-attributes/template.js

+				onChange={ ( event ) => onUpdateTemplate( event.target.value ) }
+			>
+				{ map( { '': __( 'Default template' ), ...availableTemplates }, ( templateName, templateSlug ) => (
+					<option key={ templateSlug || 'default' } value={ templateSlug }>{ templateName }</option>


Is an empty string key invalid? Wonder whether we need the || 'default' fallback.

aduth · 2018-01-24T18:22:42Z

editor/components/page-attributes/template.js

+				onChange={ ( event ) => onUpdateTemplate( event.target.value ) }
+			>
+				{ map( { '': __( 'Default template' ), ...availableTemplates }, ( templateName, templateSlug ) => (
+					<option key={ templateSlug || 'default' } value={ templateSlug }>{ templateName }</option>


Is an empty string key invalid? Wonder whether we need the || 'default' fallback.

aduth · 2018-01-24T18:22:46Z

editor/components/page-attributes/template.js

+	},
+	{
+		onUpdateTemplate( templateSlug ) {
+			return editPost( { template: templateSlug || '' } );


Should we explicitly set the template to the empty string, or is there a way we can "unset" its value to not send anything at all in the save request?

The problem with not sending anything at all in the save request is that if we had selected a template before and now we want to use the default one, if we don't send an empty string in the request e.g: don't send anything at all for the template, we will not be updating it and the template used before will continue to be used.

aduth · 2018-01-24T18:23:32Z

editor/components/page-attributes/template.js

+		};
+	},
+	{
+		onUpdateTemplate( templateSlug ) {


Could the Template suffix be considered redundant if it's clear by the component name itself that given, <PageTemplate onChange={} />, onChange would be called when the template is updated?

jorgefilipecosta · 2018-01-24T23:30:29Z

Hi @youknowriad, @aduth thank you for your reviews, most of the feedback was addressed.

pento · 2018-01-25T02:53:22Z

lib/client-assets.php

-		'blockTypes'       => $allowed_block_types,
-		'titlePlaceholder' => apply_filters( 'enter_title_here', __( 'Add title', 'gutenberg' ), $post ),
+		'alignWide'          => $align_wide || ! empty( $gutenberg_theme_support[0]['wide-images'] ), // Backcompat. Use `align-wide` outside of `gutenberg` array.
+		'availableTemplates' => wp_get_theme()->get_page_templates( $post_to_edit['id'] ),


get_page_templates() requires a WP_Post, sending it an ID does work, but it doesn't convert the ID to a WP_Post internally, for sending to the theme_{$post_type}_templates filter.

Hi @pento, thank you for the feedback, I added a call to WP_Post::get_instance so we can correctly pass a WP_Post instance to the filters.

pento · 2018-01-25T02:56:13Z

The one comment aside, I really like this approach. It gets things working now, it isn't necessary to implement the harder solutions (eg, #2086) until well after WordPress 5.0, when we might need to be able to dynamically switch which post is being edited.

aduth · 2018-01-25T11:03:17Z

editor/components/page-attributes/order.js

@@ -28,24 +28,33 @@ export function PageAttributesOrder( { onUpdateOrder, instanceId, order } ) {
 	const inputId = `editor-page-attributes__order-${ instanceId }`;

 	return (
-		<PageAttributesCheck>
-			<label htmlFor={ inputId }>
+		<div>


I would expect one or the other of a div wrapper, or array returns with key on each children, but not both. Alternatively, replacing the div with wp.element.Fragment and dropping the key. My preference would be toward one of the latter two options.

Sorry, I was switching to the array approach, I got problems in the tests and then I used another approach but I missed the revert of key changes :( I used the Fragment approach thank you for the suggestion 👍

aduth · 2018-01-25T11:06:25Z

editor/components/page-attributes/order.js

+	);
+}
+
+function PageAttributesOrderWithChecks( props ) {


Aside: Not usually a fan of multiple components defined in a single file, and this particular use makes me curious if an HoC pattern for checks could be simpler to integrate into our compose convention:

export default compose( [ ifPostTypeSupports( [ 'page-attributes' ] ), ] )( PageAttributesOrder );

I agree with the suggestion, as future work an HoC will be implemented and used in this and other cases.

gziolo · 2018-01-25T15:30:33Z

editor/components/page-attributes/order.js

+}
+
+// TODO: Refactor to use a future ifPostTypeSupports HoC
+function PageAttributesOrderWithChecks( props ) {


It probably should go to its own file and be converted to HOC when refactoring.

mcsf · 2018-01-25T15:20:59Z

lib/client-assets.php

-		'blockTypes'       => $allowed_block_types,
-		'titlePlaceholder' => apply_filters( 'enter_title_here', __( 'Add title', 'gutenberg' ), $post ),
+		'alignWide'          => $align_wide || ! empty( $gutenberg_theme_support[0]['wide-images'] ), // Backcompat. Use `align-wide` outside of `gutenberg` array.
+		'availableTemplates' => wp_get_theme()->get_page_templates( WP_Post::get_instance( $post_to_edit['id'] ) ),


Why WP_Post::get_instance? Why not get_post?

mcsf · 2018-01-25T15:30:52Z

editor/components/page-attributes/order.js

+	);
+}
+
+// TODO: Refactor to use a future ifPostTypeSupports HoC


Trying out a HoC (per #4662) sounds fine, but until then I don't think there's a point to keeping this comment.

gziolo · 2018-01-25T15:39:00Z

editor/components/page-attributes/order.js

 	);
 }

 const applyConnect = connect(
 	( state ) => {
 		return {
+			postTypeSlug: getCurrentPostType( state ),


It's not used in this file. Should we remove it or maybe move to the component that uses it?

gziolo · 2018-01-25T15:40:32Z

editor/components/page-attributes/test/check.js

 		const wrapper = shallow(
-			<PageAttributesCheck postType={ {
+			<PageAttributesCheck availableTemplates={ {} } postType={ {


Would it make sense to provide a default value for availableTemplates?

gziolo · 2018-01-25T15:46:41Z

editor/components/page-attributes/test/check.js

+	it( 'should render if page attributes support is true and no available templates exist', () => {
+		const wrapper = shallow( <PageAttributesCheck availableTemplates={ {} } postType={ postType }>content</PageAttributesCheck> );
+
+		expect( wrapper.type() ).not.toBe( null );


I'm wondering if having test like:

expect( wrapper.find( 'PageAttributesCheck' ) ).toHaveText( 'content' ); // not sure it works though :)

would better document the behavior of the component.

gziolo · 2018-01-25T16:00:50Z

editor/edit-post/sidebar/page-attributes/index.js

 	( state ) => {
 		return {
 			isOpened: isEditorSidebarPanelOpened( state, PANEL_NAME ),
+			postTypeSlug: getCurrentPostType( state ),


Can be removed, too.

OOps, it is referenced in withAPIData :)

gziolo · 2018-01-25T16:07:33Z

editor/components/page-attributes/template.js

+	} )
+);
+
+export default compose( [


You don't need to use an array with compose:

export default compose( applyConnect, applyWithContext, withInstanceId )( PageTemplate );

Is fine, too :)

I always go back and forth on this one. I was going to say that one upside of passing an array is that all functions in it can be formatted alike, notably with a trailing comma, but it seems that with our current linting + build rules the following is accepted:

export default compose( applyFoo, applyBar, // notice trailing comma )( FooBar );

TIL :)

Right, you can now add trailing comma when using ES.next 👍

gziolo · 2018-01-25T16:09:22Z

editor/components/page-attributes/template.js

+				onBlur={ onEventUpdate }
+				onChange={ onEventUpdate }
+			>
+				{ map( { '': __( 'Default template' ), ...availableTemplates }, ( templateName, templateSlug ) => (


Nit: space missing after '', before :.

If we add the space e.g make things like ''' : __( 'Default template' )' we get a linting error because of the extra space.

Skip that, I thought it is if statement :)

gziolo

It works as expected, it looks like all the major issues raised comments were addressed. In my opinion, it is ready to go. Good job! 🚢

…st types.

…or if post type has available templates. This condition is the same as the one used in core. This solves the bug of not being able to change the post template even if there are post templates available.

…me condition used in core.

This solves a bug, where Post Attributes core box was being show in "post" post type while it should be disabled.

jorgefilipecosta · 2018-01-25T23:05:28Z

Hi team, I rebased this PR to take into account the changes in https://github.com/WordPress/gutenberg/pull/4661/files. The changes are minor since the review, the most relevant changes are the addition of getCurrentPostType to the list of exposed selectors and the usage of our query HoC to retrieve the postType.

mtias · 2018-01-26T15:31:47Z

Nice :)

jorgefilipecosta self-assigned this Jan 24, 2018

jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement. labels Jan 24, 2018

youknowriad reviewed Jan 24, 2018

View reviewed changes

aduth reviewed Jan 24, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/page-template branch 5 times, most recently from 43fbd09 to fd7c2ef Compare January 24, 2018 23:26

pento reviewed Jan 25, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/page-template branch from fd7c2ef to 4aa2b75 Compare January 25, 2018 09:05

aduth reviewed Jan 25, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/page-template branch from 4aa2b75 to 0438b47 Compare January 25, 2018 11:51

jorgefilipecosta requested review from mcsf, gziolo and ellatrix January 25, 2018 12:57

aduth mentioned this pull request Jan 25, 2018

Framework: Extract "edit-post" to its own module #4661

Merged

gziolo reviewed Jan 25, 2018

View reviewed changes

mcsf reviewed Jan 25, 2018

View reviewed changes

gziolo reviewed Jan 25, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/page-template branch from 0438b47 to 8488af1 Compare January 25, 2018 15:50

gziolo reviewed Jan 25, 2018

View reviewed changes

gziolo approved these changes Jan 25, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/page-template branch 2 times, most recently from c73cfa7 to 9205627 Compare January 25, 2018 17:21

Added ability to set page/post template.

21a60c0

jorgefilipecosta force-pushed the add/page-template branch from 9205627 to b66abdd Compare January 25, 2018 22:53

jorgefilipecosta added 4 commits January 25, 2018 23:01

Used correct attribute label, instead of "Page Attributes" for all po…

31baa6b

…st types.

Updated PageAttributesCheck to show if post supports page attributes …

8a01337

…or if post type has available templates. This condition is the same as the one used in core. This solves the bug of not being able to change the post template even if there are post templates available.

Only show order input field if post type supports page attributes, sa…

9c1604f

…me condition used in core.

Added pageparentdiv to the core_side_meta_boxes array.

9af433d

This solves a bug, where Post Attributes core box was being show in "post" post type while it should be disabled.

jorgefilipecosta force-pushed the add/page-template branch from b66abdd to 9af433d Compare January 25, 2018 23:02

mcsf approved these changes Jan 26, 2018

View reviewed changes

jorgefilipecosta merged commit 772176b into master Jan 26, 2018

jorgefilipecosta deleted the add/page-template branch January 26, 2018 11:34

mtias mentioned this pull request Feb 7, 2018

Scope & Features: MVP #4894

Closed

9 tasks

samikerb mentioned this pull request Jul 29, 2020

correct bad condition #24273

Closed

3 tasks


		// Disable reason: A select with an onchange throws a warning

		/* eslint-disable jsx-a11y/no-onchange */

Add ability to set page/post template #4662

Add ability to set page/post template #4662

Conversation

jorgefilipecosta commented Jan 24, 2018 • edited Loading

Description

How Has This Been Tested?

Types of changes

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 Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pento commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jan 25, 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

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 Jan 25, 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

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jan 25, 2018

mtias commented Jan 26, 2018

jorgefilipecosta commented Jan 24, 2018 •

edited

Loading

jorgefilipecosta commented Jan 24, 2018 •

edited

Loading

jorgefilipecosta Jan 25, 2018 •

edited

Loading

gziolo Jan 25, 2018 •

edited

Loading