SPT: Set template title in large preview #39831

retrofox · 2020-03-03T15:20:58Z

Changes proposed in this Pull Request

This PR improves how the template title is rendered into the preview. Now, it's a core/heading h1 block. What this PR proposes is to use the <PostTitle /> core editor component.

The only tricky part to deal is that this component gets the title from the post data. This is why we've created this PR in the core. In the meanwhile, however, we can go ahead in setting the template title using DOM manipulation. Once the core PR is ready 🤞, we can update and simplify the current approach.

Testing instructions

Apply the patch
Check how the title is rendered in the large preview box
Compare with editor-canvas
Test in different themes in order to detect format variations such as alignment, font-size, etc
Compare with master branch

Mayhood

before

after

Fixes #39805

It implements a hacky solution to propagate the template title to the <PostTitle /> component, since it isn't possible to do it in thr current implementation of the component. I've created a PR, but in the meanwhile we can take this patch in order to give a quick solution. We can iterate later once the code PR is ship? WordPress/gutenberg#20609

matticbot · 2020-03-03T15:21:00Z

Test live: https://calypso.live/?branch=update/spt-template-title-in-large-preview

matticbot · 2020-03-03T19:19:52Z

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-03T19:31:20Z

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 D39778-code before merging this PR. Thank you!

jeryj

Overall, this seems like a clever solution for being able to use the <PostTitle /> while waiting on an easier way to pass a string to the <PostTitle /> component 🙌

The only functional things I came across when testing in Chrome, Safari, and Firefox in multiple themes were:

There's extra spacing underneath of the preview title compared to master
Previously, the title was within the preview blocks, which meant the margins between blocks were collapsed. This title is outside of that block list, so the margins don't collapse and there's extra space.

This branch:

Master:

Safari shows the preview title in a faded color. master shows it in black, as expected.

...editing-plugin/starter-page-templates/page-template-modal/components/block-iframe-preview.js

...l-site-editing-plugin/starter-page-templates/page-template-modal/components/block-preview.js

...ng-plugin/starter-page-templates/page-template-modal/components/template-selector-preview.js

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

getdave

Thanks for this PR. I haven't manually tested this yet. Noticed some minor points when reviewing the code which I'm happy to discuss and help clarify.

My principle concern here is that we're introducing a hack which breaks React's data flow model. As soon as we start reaching into the DOM and tinkering we're introducing the very side effects that React is designed to avoid. This could lead to unintended consequences especially given that we're relying on a Core component which is subject to change in it's DOM structure...etc. In some ways the DOM isn't part of the component's public API so it is subject to change without warning which could break this implementaiton.

That said, I fully appreciate that this change is being viewed as a temporary workaround. That's ok if it truly is temporary. However, I have concerns about landing this unless we are confident that:

the Core PR to set the title directly as a prop will be accepted - can we get a Core team member to provide some indication of this?
our team will have time allocated in a future Iteration to come back and remove this hack with the "proper" solution

Thanks for exploring this and doing such a thorough deep dive 👍

...editing-plugin/starter-page-templates/page-template-modal/components/block-iframe-preview.js

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

@getdave

…emplates/page-template-modal/components/block-iframe-preview.js Improve jsdoc. Props to @getdave :-) Co-Authored-By: Dave Smith <[email protected]>

@getdave

…emplates/page-template-modal/components/block-iframe-preview.js Improve jsdoc. Props to @getdave Co-Authored-By: Dave Smith <[email protected]>

…emplates/page-template-modal/styles/starter-page-templates-editor.scss Set the CSS class name more targetted to the SPT scope. Co-Authored-By: Dave Smith <[email protected]>

retrofox · 2020-03-04T13:27:01Z

...That said, I fully appreciate that this change is being viewed as a temporary workaround.

Totally agree. This is something that we should try to achieve with Frame v2

retrofox · 2020-03-04T13:28:25Z

our team will have time allocated in a future Iteration to come back and remove this hack with the "proper" solution

I don't think so in the short term. I'd like to continue with this in my 20% time, though.

getdave

This is looking much better. Thanks for all the improvements 👍

One thing I'd suggest is that we create a Calypso Issue now in our backlog to track the task for updating to use the update Core component if your Core PR to allow for a placeholder title prop on the PostTitle component is accepted.

...editing-plugin/starter-page-templates/page-template-modal/components/block-iframe-preview.js

getdave · 2020-03-04T13:30:03Z

...l-site-editing-plugin/starter-page-templates/page-template-modal/components/block-preview.js


 // Exists as a pass through component to simplify automatted testing of
 // components which need to `BlockEditorProvider`. Setting up JSDom to handle
 // and mock the entire Block Editor isn't useful and is difficult for testing.
 // Therefore this component exists to simplify mocking out the Block Editor
 // when under test conditions.
 export default function( { blocks, settings, recomputeBlockListKey } ) {
+	/* eslint-disable wpcalypso/jsx-classname-namespace */


If we've updated to block-iframe-preview__template-title do we need the eslint disable? It's better to keep these kinds of warnings where possible as (despite the "noise") they are the first line of defence against bugs.

...editing-plugin/starter-page-templates/page-template-modal/components/block-iframe-preview.js

retrofox · 2020-03-04T13:49:56Z

Safari shows the preview title in a faded color. master shows it in black, as expected.

Can we create a new for this one?

retrofox added 4 commits March 3, 2020 11:39

spt: refact adding setTemplateTitle

e039d12

spt: apply delay for the first-rendering cycle

1bce59e

spt: remove core/heading teplate title from blocks

bf74601

retrofox requested a review from ajaykj March 3, 2020 15:24

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

retrofox mentioned this pull request Mar 3, 2020

SPT: Re implement template post title. #39809

Closed

spt: add padding top to the template title

99a1a5b

spt: add missing template title classname

5c8c20a

retrofox requested a review from a team March 3, 2020 19:30

retrofox marked this pull request as ready for review March 3, 2020 19:30

jeryj requested changes Mar 3, 2020

View reviewed changes

getdave requested changes Mar 4, 2020

View reviewed changes

retrofox and others added 5 commits March 4, 2020 08:00

Update apps/full-site-editing/full-site-editing-plugin/starter-page-t…

eef39d6

…emplates/page-template-modal/components/block-iframe-preview.js Improve jsdoc. Props to @getdave :-) Co-Authored-By: Dave Smith <[email protected]>

Update apps/full-site-editing/full-site-editing-plugin/starter-page-t…

21b8420

…emplates/page-template-modal/components/block-iframe-preview.js Improve jsdoc. Props to @getdave Co-Authored-By: Dave Smith <[email protected]>

Update apps/full-site-editing/full-site-editing-plugin/starter-page-t…

be70a3b

…emplates/page-template-modal/styles/starter-page-templates-editor.scss Set the CSS class name more targetted to the SPT scope. Co-Authored-By: Dave Smith <[email protected]>

spt: reduce redice hook effect requeriments

5769035

spt: remove setTimeout hack

29c39e5

WunderBart mentioned this pull request Mar 4, 2020

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

Merged

retrofox added 2 commits March 4, 2020 09:03

spt: always render preview content

e90012d

spt: remove unneeded id attribute

83e958c

retrofox removed the request for review from ajaykj March 4, 2020 12:26

WunderBart force-pushed the update/spt-template-title-in-large-preview branch from 57b43bd to 8e087e4 Compare March 4, 2020 12:40

spt: removes CSS hack

437b03c

retrofox force-pushed the update/spt-template-title-in-large-preview branch from 8e087e4 to 437b03c Compare March 4, 2020 13:23

getdave approved these changes Mar 4, 2020

View reviewed changes

getdave added the [Goal] Page Templates label Mar 4, 2020

getdave assigned retrofox Mar 4, 2020

spt: improve doc

a893b50

WunderBart added the [Goal] Full Site Editing label Mar 4, 2020

retrofox merged commit dccfbd3 into master Mar 4, 2020

retrofox deleted the update/spt-template-title-in-large-preview branch March 4, 2020 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPT: Set template title in large preview #39831

SPT: Set template title in large preview #39831

retrofox commented Mar 3, 2020 •

edited

Loading

matticbot commented Mar 3, 2020

matticbot commented Mar 3, 2020

matticbot commented Mar 3, 2020

jeryj left a comment •

edited by retrofox

Loading

getdave left a comment •

edited

Loading

retrofox commented Mar 4, 2020 •

edited

Loading

retrofox commented Mar 4, 2020

getdave left a comment

getdave Mar 4, 2020

retrofox commented Mar 4, 2020

SPT: Set template title in large preview #39831

SPT: Set template title in large preview #39831

Conversation

retrofox commented Mar 3, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Mayhood

matticbot commented Mar 3, 2020

matticbot commented Mar 3, 2020

matticbot commented Mar 3, 2020

jeryj left a comment • edited by retrofox Loading

Choose a reason for hiding this comment

getdave left a comment • edited Loading

Choose a reason for hiding this comment

retrofox commented Mar 4, 2020 • edited Loading

retrofox commented Mar 4, 2020

getdave left a comment

Choose a reason for hiding this comment

getdave Mar 4, 2020

Choose a reason for hiding this comment

retrofox commented Mar 4, 2020

retrofox commented Mar 3, 2020 •

edited

Loading

jeryj left a comment •

edited by retrofox

Loading

getdave left a comment •

edited

Loading

retrofox commented Mar 4, 2020 •

edited

Loading