SPT: Re implement template post title. #39809

retrofox · 2020-03-02T12:52:46Z

Changes proposed in this Pull Request

A few days ago we changed the way to render the template title, inserting a core/heading block among all templates blocks there. It simplifies the way to compute the scaling for the title.
This PR keeps iterating over the same feature, creating a new a8c/spt-template-post-title and using it instead of the core/heading one. It allows inheriting theme styles such as font-size, alignment, etc.
For instance, testing with Mayland theme:

Editor Canvas

After (correct title alignment)

Before (wrong title alignment)

Testing instructions

Apply these changes in your testing environment.
Test how the template post title looks compared one the template is inserted in the editor-canvas.

Fixes #39805

matticbot · 2020-03-02T12:52:48Z

Test live: https://calypso.live/?branch=update/spt-add-post-title-block

matticbot · 2020-03-02T13:04:14Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot · 2020-03-02T15:43:19Z

Caution: This PR has changes that must be merged to WordPress.com
Hello retrofox! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39685-code before merging this PR. Thank you!

obenland · 2020-03-02T15:49:57Z

What's the benefit of using a custom block? why can't the heading block be centered?

retrofox · 2020-03-02T16:00:43Z

What's the benefit of using a custom block? why can't the heading block be centered?

The idea behind this PR is copying the core/post-title behavior. This block implements a proper markup (also adding CSS classes) which gives the theme the ability to apply custom style to the title.

For instance, Twenty Twenty sets the title at the center, as while as Mayland does it at the left. Unfortunately core/post-title is an experiment block so far and even when it rolls out we have to keep in mind that it couldn' t work for some Atomic sites.

getdave

Nice job on this 👍

Noticed a few things on first code pass.

getdave · 2020-03-02T16:32:07Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

+ * This block will be unregistered once the template is inserted,
+ * the component unmounted, modal closed, etc...
+ */
+registerBlockType( 'a8c/spt-template-post-title', {


I think maybe we should add some context as to why we're adding a custom block here. You provided reasoning in the PR comments which would work well.

getdave · 2020-03-02T16:32:31Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

+ * the component unmounted, modal closed, etc...
+ */
+registerBlockType( 'a8c/spt-template-post-title', {
+	title: __( 'SPT Template post title' ),


Do we need a 2nd arg here of full-site-editing?

getdave · 2020-03-02T16:32:49Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

+ */
+registerBlockType( 'a8c/spt-template-post-title', {
+	title: __( 'SPT Template post title' ),
+	description: __( 'Template post title.' ),


Do we need a 2nd arg here of full-site-editing?

eg:

__( 'Template post title.', 'full-site-editing` ),

getdave · 2020-03-02T16:33:03Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

+		return (
+			<div className="editor-post-title">
+				<div className="wp-block editor-post-title__block">
+					<div>


What's the empty <div> for?

getdave · 2020-03-02T16:33:53Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

@@ -51,10 +90,8 @@ class PageTemplateModal extends Component {
 							 * It will be remove when inserting the template


Suggested change

* It will be remove when inserting the template

* It will be removed when the template's blocks are inserted into the editor.

getdave · 2020-03-02T16:35:35Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

@@ -162,6 +205,10 @@ class PageTemplateModal extends Component {
 		trackSelection( this.props.segment.id, this.props.vertical.id, slug );
 		this.props.saveTemplateChoice( slug );

+		if ( this.props.isPostTitleBlockRegistered ) {
+			unregisterBlockType( 'a8c/spt-template-post-title' );


As above. Should we also be tying the registration of this block to the component mount hook?

getdave · 2020-03-02T16:35:47Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

@@ -247,6 +294,10 @@ class PageTemplateModal extends Component {
 	};

 	closeModal = event => {
+		if ( this.props.isPostTitleBlockRegistered ) {
+			unregisterBlockType( 'a8c/spt-template-post-title' );


...and again.

getdave · 2020-03-02T16:36:05Z

...-plugin/starter-page-templates/page-template-modal/styles/starter-page-templates-editor.scss

@@ -439,14 +439,14 @@ body.admin-bar:not( .is-fullscreen-mode ) .page-template-modal-screen-overlay {
 	}

 	// Manual CSS Overrides. Remove after better solutions are in place.
-	
+


Random space

getdave · 2020-03-02T16:36:38Z

...-plugin/starter-page-templates/page-template-modal/styles/starter-page-templates-editor.scss

 	// Removes empty paragraph placeholders, i.e. "Write Title..."
 	[data-type='core/paragraph'] [data-rich-text-placeholder] {
 		display: none;
 	}

 	/*
-	* Fixes jetpack .wp-block-jetpack-slideshow styles, as the /wp-content/plugins/jetpack/_inc/blocks/vendors~swiper.[hash].css 
+	* Fixes jetpack .wp-block-jetpack-slideshow styles, as the /wp-content/plugins/jetpack/_inc/blocks/vendors~swiper.[hash].css
 	* file is loaded on block insert, not on page load. After the iframe is grabbing these styles, we can remove this code.


Suggested change

* file is loaded on block insert, not on page load. After the iframe is grabbing these styles, we can remove this code.

* file is loaded on block insert, not on page load. After the iframe has loaded these styles, we can remove this code.

getdave · 2020-03-02T16:37:34Z

...-plugin/starter-page-templates/page-template-modal/styles/starter-page-templates-editor.scss

@@ -474,4 +474,13 @@ body.admin-bar:not( .is-fullscreen-mode ) .page-template-modal-screen-overlay {
 			margin-bottom: 0;
 		}
 	}
+
+	// Adjust template title.
+	.editor-post-title__input {


Is this class added so it intentionally inherits existing styles? If not and these styles are bespoke to this one block then is the classname specific enough?

jeryj · 2020-03-02T16:55:02Z

...-plugin/starter-page-templates/page-template-modal/styles/starter-page-templates-editor.scss

+		overflow: hidden;
+		overflow-wrap: break-word;
+		resize: none;
+		max-height: 80px;


This 80px seems risky. Why does it need to be 80px, and are we positive the title will never be multi-line? If the title goes multi-line, it would be hidden due to the max-height value.

jeryj · 2020-03-02T16:58:36Z

...ll-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/index.js

+		if ( this.props.isPostTitleBlockRegistered ) {
+			unregisterBlockType( 'a8c/spt-template-post-title' );
+		}
+


Why does this need to be unregistered anytime a template is selected?

jeryj · 2020-03-02T17:10:37Z

Added a couple small comments. I don't feel qualified to review the actual registering/unregistering implementation code, as I haven't worked with that before. That said, I did a functional review, testing the implementation with three themes: 2020, Coutoire, and Varia. The implementation worked as expected in each of them 👍

jeryj · 2020-03-02T17:15:24Z

Actually, I missed one thing. Left aligned title have extra spacing on the left that is not there in the editor or front-end:

Coutoire Menu template in SPT Preview:

In Editor:

retrofox · 2020-03-03T15:26:34Z

Closing in favor of #39831

retrofox requested a review from a team as a code owner March 2, 2020 12:52

retrofox added 5 commits March 2, 2020 12:39

spt: add a8c/spt-template-post-title custom block

bb65981

spt: unregister post title block when tpl selects

f386164

spt: ensuring unregister register block

93f2e21

spt: adjust template post title size.

c64ff84

spt: tweak template post title height

941bf13

retrofox force-pushed the update/spt-add-post-title-block branch from fd0f4ff to 941bf13 Compare March 2, 2020 15:40

retrofox self-assigned this Mar 2, 2020

retrofox added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Mar 2, 2020

getdave reviewed Mar 2, 2020

View reviewed changes

jeryj reviewed Mar 2, 2020

View reviewed changes

jeryj mentioned this pull request Mar 2, 2020

SPT: Make page title visibility depend on theme setting #39702

Merged

retrofox closed this Mar 3, 2020

sirreal deleted the update/spt-add-post-title-block branch December 4, 2020 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPT: Re implement template post title. #39809

SPT: Re implement template post title. #39809

retrofox commented Mar 2, 2020

matticbot commented Mar 2, 2020

matticbot commented Mar 2, 2020

matticbot commented Mar 2, 2020

obenland commented Mar 2, 2020

retrofox commented Mar 2, 2020 •

edited

Loading

getdave left a comment •

edited

Loading

getdave Mar 2, 2020 •

edited

Loading

getdave Mar 2, 2020

getdave Mar 2, 2020

getdave Mar 2, 2020

getdave Mar 2, 2020

getdave Mar 2, 2020

getdave Mar 2, 2020

getdave Mar 2, 2020

getdave Mar 2, 2020

getdave Mar 2, 2020

jeryj Mar 2, 2020

jeryj Mar 2, 2020

jeryj commented Mar 2, 2020

jeryj commented Mar 2, 2020

retrofox commented Mar 3, 2020

		@@ -51,10 +90,8 @@ class PageTemplateModal extends Component {
		* It will be remove when inserting the template

	* It will be remove when inserting the template
	* It will be removed when the template's blocks are inserted into the editor.

		@@ -439,14 +439,14 @@ body.admin-bar:not( .is-fullscreen-mode ) .page-template-modal-screen-overlay {
		}

		// Manual CSS Overrides. Remove after better solutions are in place.

	* file is loaded on block insert, not on page load. After the iframe is grabbing these styles, we can remove this code.
	* file is loaded on block insert, not on page load. After the iframe has loaded these styles, we can remove this code.

SPT: Re implement template post title. #39809

SPT: Re implement template post title. #39809

Conversation

retrofox commented Mar 2, 2020

Changes proposed in this Pull Request

Testing instructions

matticbot commented Mar 2, 2020

matticbot commented Mar 2, 2020

matticbot commented Mar 2, 2020

obenland commented Mar 2, 2020

retrofox commented Mar 2, 2020 • edited Loading

getdave left a comment • edited Loading

Choose a reason for hiding this comment

getdave Mar 2, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeryj commented Mar 2, 2020

jeryj commented Mar 2, 2020

retrofox commented Mar 3, 2020

retrofox commented Mar 2, 2020 •

edited

Loading

getdave left a comment •

edited

Loading

getdave Mar 2, 2020 •

edited

Loading