FSE: Add Current layout to starter page templates page layout picker when editing a homepage #43980

andrewserong · 2020-07-08T08:42:03Z

Changes proposed in this Pull Request

Add Current layout option to page layout picker when a user is editing / updating a homepage

Still to do

Switch on dynamic previews for just the Current button, and see if that works instead of having a screenshot or empty square for this button
The current option shouldn't display if this is a brand new page

Screenshot

Testing instructions

Spin up a local development environment (e.g. go to the apps/full-site-editing directory and run yarn wp-env start
Run the local dev environment for FSE (e.g. fo to the apps/full-site-editing directory and run yarn dev or if you want to sync to your sandbox, run yarn dev --sync)
In your WP install, set a page to be the front page in the customizer
Go to edit that page, and then add the ?new-homepage query param so that you open up the page layout selector for a homepage, where you should then see the Current layout option. E.g. for my own local test site, the URL is http://localhost:4013/wp-admin/post.php?post=2&action=edit&new-homepage
With Current layout selected, you should see a preview of the current blocks in your page, and the page title should be correct in the preview
Try selecting different page layouts — these should all replace the page's content, however if you select Current layout, it should not replace the page content.

Related to: #43933

matticbot · 2020-07-08T08:42:05Z

Test live: https://calypso.live/?branch=update/fse-starter-page-templates-to-include-current-layout

matticbot · 2020-07-08T08:49:03Z

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.

a8ci18n · 2020-07-08T08:49:07Z

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

+								{ this.props.isFrontPage &&
+									this.renderTemplatesList(
+										currentTemplate,
+										__( 'Current', 'full-site-editing' )


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 100 times:
translate( 'Current' ) ES Score: 10

matticbot · 2020-07-08T08:50:25Z

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D46093-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

Addison-Stavlo

Updated this to:

Show the dynamic preview for the 'current' layout tab button.
Skip the 'current' section if there are no blocks on the page. (empty/new page scenario)

Unfortunately, In implementing the preview I have also introduced a bug/error that I am having a hard time tracking down. Sometimes it happens right when we open the layout picker, but it is mostly reproducible when we have the 'current' tab selected and press "Tab" to move to the next in an a11y style flow. The editor breaks and there is an error in the console about using getBoundingClientRect of undefined:

I have not yet been able to pinpoint this or work around it.

On another note, if we do get this working in an acceptable manner I wonder if we want this available for all pages, not just the homepage? 🤔

Addison-Stavlo · 2020-07-08T20:54:22Z

...iting-plugin/starter-page-templates/page-template-modal/components/template-selector-item.js

-		<Disabled>
-			<BlockIframePreview blocks={ blocks } viewportWidth={ 960 } />
-		</Disabled>
+		<BlockIframePreview blocks={ blocks } viewportWidth={ 960 } />


The Disabled component here causes 2 problems:

It styles push the preview below/outside the tab window.

It has zero height, which in turn causes the scaling to give the preview 0 height in:

wp-calypso/apps/full-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/components/block-iframe-preview.js

Line 119 in a983b61

const height = parentNode.offsetHeight / scale;

Removing this allows the preview to be viewable. Similarly, there is already a Disabled component further down in the chain in:

wp-calypso/apps/full-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/components/block-preview.js

Line 25 in a983b61

<Disabled key={ recomputeBlockListKey }>

So Im not sure if its necessary here. If it is, we may need to work-around the default component styles and scaling implementation to account for out purposes here.

I am finding the noted getBoundingClientRect error both with and without the Disabled component, it seems to be specific to using the BlockIframePreview here. 🤔

andrewserong · 2020-07-09T08:44:11Z

Nice work on this @Addison-Stavlo — I've done a small update to move the blocks for the isCurrentPreview to within the renderTemplatesList function. Because getBlocksByTemplateSlugs is memoized, it was returning no blocks for current if the selector hadn't finished resolving before it was first called. This meant that I was intermittently seeing there was no "Current" layout option because for some reason in my local environment, it was taking a little longer to resolve that selector. This change isn't quite as neat, but means that when the selector updates, the template list should display correctly.

I looked into the getBoundingClientRect issue and it looks or feels like there's a Popover somewhere that's trying re-adjust its size, and I'm wondering if somehow the block provider expects there to be a Popover.Slot — or because our parent document's block provider has one, it's somehow expecting something like that here? I found a workaround by adding tab-index=-1 to the iframe, to prevent the user from tabbing into it, which seems to suppress that error, and enable tabbing past the Current layout option. I can see the issue is still present in the main preview if you click into it and tab around, but that's separate from our proposed changes here.

So, it looks like we might be okay without having to add back in Disabled?

andrewserong · 2020-07-09T09:53:31Z

Oh, and yes, I agree, if we get this working acceptably, I think it’d be great to include the current layout option for when a page already exists and someone is looking to swap layouts.

Another thought re: the getBoundingClientRect error being thrown while tabbing within a preview — it’s almost like it’s trying to open the toolbar pop up for each block or something like that? More broadly it feels like this approach to previews has a lot more going on than we might need to a non-interactive preview, but I see it’s very similar to the block pattern previews in Gutenberg. Just a bit of food for thought if we get the time to rethink these previews in the future!

Addison-Stavlo · 2020-07-09T13:31:50Z

Very nice, the tabIndex there solves it for the thumbnail preview. 😅 I was trying tabIndex in a couple places but I must have goofed on trying the iframe element itself.

Its odd how that doesn't solve the same issue for the large preview though 🤔 although thats less of a concern for this PR I think. (investigate/created #44038 regarding that)

Addison-Stavlo · 2020-07-09T19:49:01Z

Testing this some more it seems to work well! Especially considering that the error we have encountered seems to be present on master and is a recent problem stemming from core Gutenberg after 8.3.

I removed the check for isFrontPage so the current content option should be available for all pages. Il set this to 'ready for review' (I hope thats ok, or maybe there was something else you were hoping to add or change here?), but it seems we will need to approve this and deploy an FSE plugin release for it before we can merge #43933

a8ci18n · 2020-07-09T19:51:29Z

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

@@ -413,6 +438,11 @@ class PageTemplateModal extends Component {
 					) : (
 						<>
 							<form className="page-template-modal__form">
+								{ this.renderTemplatesList(
+									currentTemplate,
+									__( 'Current', 'full-site-editing' )


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 100 times:
translate( 'Current' ) ES Score: 10

alshakero · 2020-07-14T12:50:01Z

Hi Andy and Addison,

The current option shouldn't display if this is a brand-new page

I implemented this one, although I'm not entirely sure about the validity of the criteria I used to tell if the page is new. I took inspiration from this line. And it seems to work as expected.

As for

Switch on dynamic previews for just the Current button, and see if that works instead of having a screenshot or empty square for this button

This seems to be already done. I changed the text color to red and the button reflected that alright:

What was not done though is that the big preview isn't showing the "current". It didn't show "Current". So I changed getDefaultSelectedTemplate to return Current when the page isn't new. I'm not entirely confident about this change though. Would appreciate some feedback.

Thanks!

a8ci18n · 2020-07-14T12:54:17Z

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

-			return DEFAULT_HOMEPAGE_TEMPLATE;
-		}
-		return blankTemplate;
+		// if the page isn't new, select "Current" as the default template


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 100 times:
translate( 'Current' ) ES Score: 10

Addison-Stavlo

Thanks for jumping in on this one @alshakero!

The current option shouldn't display if this is a brand-new page

I implemented this one, although I'm not entirely sure about the validity of the criteria I used to tell if the page is new. I took inspiration from this line. And it seems to work as expected.

Switch on dynamic previews for just the Current button, and see if that works instead of having a screenshot or empty square for this button

This seems to be already done.

My apologies as I forgot to update the original comment to reflect the state of these TODOs. We did turn the dynamic preview on for that in one of the previous commits. I also implemented a check to skip this 'current' option for blank pages (should probably be good enough for the new-page case? see below).

Addison-Stavlo · 2020-07-14T17:41:12Z

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

+		// Skip rendering current preview if there is no page content.
+		if ( isCurrentPreview && ! blocksByTemplateSlug.current?.length ) {
+			return null;
+		}


This is what I had added as the condition to hide the 'Current' section on a new page. I was assuming that a new page would always have no content, or that if it did have content we wouldn't want to hide 'Current' ? 🤔 Similarly if it is not a new page and has no content, we don't need the 'current' section.

I think these are valid assumptions. We probably don't need "Current" for any empty page. New or not.

alshakero · 2020-07-15T11:25:08Z

My apologies as I forgot to update the original comment to reflect the state of these TODOs.

No worries. I probably should have paid more attention to the convo here. I reversed my redundant check and only kept the change that renders the "current" contents in the preview iframe.

I tested and all works as intended.

ockham · 2020-08-05T14:07:54Z

Going to rebase.

Copons · 2020-08-24T18:30:34Z

Rebased again as there was an issue in an unrelated sub-plugin that were crashing wp-admin. 🙂

Copons · 2020-08-24T18:48:49Z

This looked good to me and tested fine...
But then I really had to go in and replace an unnecessary let with a const, so now it feels awkward to self-approve the PR. 😅

~~I'll leave it around for a bit longer in case someone has something to add, otherwise I think this is ready to merge.~~

EDIT:
Just noticed the "Do not merge" label and a missing item in the checklist:

Switch on dynamic previews for just the Current button, and see if that works instead of having a screenshot or empty square for this button

@andrewserong, @Addison-Stavlo: AFAICS this has already been sorted out in 7019cbe.
Is there anything else that's missing?

Copons · 2020-08-25T15:15:00Z

Rebased after #44501

Addison-Stavlo · 2020-08-26T00:42:40Z

But then I really had to go in and replace an unnecessary let with a const

You couldn't just let it be? 🤣

so now it feels awkward to self-approve the PR

lol, yeah it feels awkward for me to self-approve as well since I worked on it some previously.

✅ Consider your changes in c3df4b0 approved via this comment if it makes approving the rest feel less awkward. 😁

Is there anything else that's missing?

I can't think of anything beyond what Andrew just said above. Thanks for picking this up!

apps/editing-toolkit/editing-toolkit-plugin/starter-page-templates/page-template-modal/index.js

Copons · 2020-08-26T16:22:41Z

Other than a relatively minor doubt, I'm seeing two visual glitches:

When the Current template is autoselected, the post title doesn't resize appropriately:

This is caused by the PostTitle component failing to resize when it obtains the value via DOM manipulation.
The code there mentions a Core PR that will enable us to set the title directly via props instead.
It seems it would fix this issue, but meanwhile we are stuck with the DOM manipulation.

One way we could fix this is to replace <PostTitle /> with the actual rendered code of the component.

<div class="wp-block editor-post-title editor-post-title__block">
	<div class="editor-post-title__input">{ title }</div>
</div>

It is slightly fragile, but we might want to assume that the PostTitle rendered content won't change much before WordPress/gutenberg#20609 is merged. 🤔

When opening the template selector with Current autoselected, there's a very very brief FOUC.

It's likely caused by copyStylesToIframe taking more time than the preview to be populated with blocks.

Given the flash is barely noticeable, and the fix could be complicated, we should probably rather tackle it in a follow up.

Addison-Stavlo · 2020-08-26T19:42:11Z

One way we could fix this is to replace with the actual rendered code of the component.

I think that seems reasonable for the time being? the DOM manipulation is fragile as-is, I don't think this is going to make it much more fragile in comparison? It will be great when we can get rid of the DOM manipulation all together.

Given the flash is barely noticeable, and the fix could be complicated, we should probably rather tackle it in a follow up.

That sounds fair as well. Although are there circumstances where this would take much longer and be very noticeable? 🤔

Copons · 2020-08-27T14:13:08Z

That sounds fair as well. Although are there circumstances where this would take much longer and be very noticeable? 🤔

It's definitely noticeable on slow connections.
I'd argue that slow connections are already used to FOUCs, to be honest.

I've also given more thought about replacing PostTitle with its rendered content, and I think that too should be tackled separately.
The title handling affects various parts of the modal, and removing the component means also deleting a good amount of code, which can lead to regressions and whatnot.

Given that this PR works, and the 2 updates are normal (the title cut off in some cases) or low (the FOUC) priority, I'd say let's merge this and try to tackle the updates before the next Editing Toolkit release.

Addison-Stavlo · 2020-08-28T16:16:54Z

I'd say let's merge this and try to tackle the updates before the next Editing Toolkit release.

That sounds fair.

Other possible ideas for the title issue - Either remove it from from the preview when it is showing 'current' or maybe just hardcode it as 'Current Layout'. When you select other layouts to look at previews, the title it shows ends up being title of the preview and not the actual post title anyways? 🤔

david-szabo97 · 2020-09-02T11:23:23Z

Can't we hack the PostTitle for now? I know it's very hacky but might be a good temporary solution to add a style to override the height of the post title.

iframeRef.current.contentDocument.head.innerHTML += '<style>.editor-post-title .editor-post-title__input { height: auto !important; }</style>';

The current layout never shows up in the preview. Is it just happening for me? 😕

I noticed the currentBlocks is an empty array []. Since we memoize the getBlocksForPreview function, it always stay an empty array. A quick hack would be to clear the cache if the currentBlocks is loaded:

const currentBlocksPreviewCache = this.getBlocksForPreview.cache.get( 'current' );
if ( currentBlocksPreviewCache && currentBlocksPreviewCache.length !== currentBlocks.length ) {
	this.getBlocksForPreview.cache.delete( 'current' );
}

Addison-Stavlo · 2020-09-03T14:47:36Z

The current layout never shows up in the preview. Is it just happening for me?

The current layout seems to show up for me, I wonder what the difference here is 🤔 . Either way, if clearing the cache fixes it on your end that seems reasonable to me.

Can't we hack the PostTitle for now?

Yeah, that seems fair. Injecting the style like that should fix it without introducing any of the negatives @Copons was mentioning above.

Addison-Stavlo

Tested with the most recent changes and it works well on my end. The current layout is displayed and the title issue is resolved.

…editing a homepage

…yTemplateSlugs is memoized

andrewserong self-assigned this Jul 8, 2020

andrewserong added DO NOT MERGE [Status] In Progress labels Jul 8, 2020

a8ci18n reviewed Jul 8, 2020

View reviewed changes

Addison-Stavlo self-assigned this Jul 8, 2020

Addison-Stavlo reviewed Jul 8, 2020

View reviewed changes

Addison-Stavlo mentioned this pull request Jul 9, 2020

FSE: When switching to an auto loading homepage theme, ensure Edit Homepage link opens up the page layout picker in Atomic / Jetpack sites #43933

Merged

Addison-Stavlo mentioned this pull request Jul 9, 2020

SPT Preview - editor crashes with 'getBoundingClientRect of undefined' error on 8.5-alpha+ #44038

Closed

Addison-Stavlo marked this pull request as ready for review July 9, 2020 19:49

a8ci18n reviewed Jul 9, 2020

View reviewed changes

Addison-Stavlo changed the title ~~WIP: FSE: Add Current layout to starter page templates page layout picker when editing a homepage~~ FSE: Add Current layout to starter page templates page layout picker when editing a homepage Jul 9, 2020

a8ci18n reviewed Jul 14, 2020

View reviewed changes

Addison-Stavlo reviewed Jul 14, 2020

View reviewed changes

alshakero force-pushed the update/fse-starter-page-templates-to-include-current-layout branch from 880d697 to 1ac7213 Compare July 15, 2020 11:21

ockham force-pushed the update/fse-starter-page-templates-to-include-current-layout branch from 1ac7213 to ac2eeb3 Compare August 5, 2020 14:08

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 24, 2020

Copons force-pushed the update/fse-starter-page-templates-to-include-current-layout branch from ac2eeb3 to 6e70f45 Compare August 24, 2020 18:29

Copons self-assigned this Aug 24, 2020

Copons mentioned this pull request Aug 25, 2020

Bump WordPress.com Editing Toolkit plugin to v2.0 #45186

Merged

Copons reviewed Aug 26, 2020

View reviewed changes

apps/editing-toolkit/editing-toolkit-plugin/starter-page-templates/page-template-modal/index.js Show resolved Hide resolved

Copons force-pushed the update/fse-starter-page-templates-to-include-current-layout branch from c3df4b0 to 9673db5 Compare August 27, 2020 13:39

Addison-Stavlo approved these changes Sep 3, 2020

View reviewed changes

andrewserong and others added 10 commits September 3, 2020 18:22

Add Current layout to starter page templates page layout picker when …

535fd5d

…editing a homepage

current tab dynamic preview

61793a6

remove current preview if no page content

133c4e3

Move current preview blocks to renderTemplatesList because getBlocksB…

4edd199

…yTemplateSlugs is memoized

Update snapshot to reflect tab-index change

221d775

remove isFrontPage qualifier

3a3ee0b

Show current content in the preview window

37a1cf8

Replace an unnecessary let with a const

8c36897

Add quick CSS fix to make sure post title is the correct height

6437939

Clear cache for current preview if currentBlocks change

bdbfa60

david-szabo97 force-pushed the update/fse-starter-page-templates-to-include-current-layout branch from 6ad9c0f to bdbfa60 Compare September 3, 2020 16:22

david-szabo97 merged commit 0e11eee into master Sep 4, 2020

david-szabo97 deleted the update/fse-starter-page-templates-to-include-current-layout branch September 4, 2020 07:08

matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 4, 2020

david-szabo97 added Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin and removed DO NOT MERGE labels Sep 4, 2020

razvanpapadopol mentioned this pull request Sep 10, 2020

Bump WordPress.com Editing Toolkit plugin to v2.3 #45536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FSE: Add Current layout to starter page templates page layout picker when editing a homepage #43980

FSE: Add Current layout to starter page templates page layout picker when editing a homepage #43980

andrewserong commented Jul 8, 2020 •

edited by Copons

Loading

matticbot commented Jul 8, 2020

matticbot commented Jul 8, 2020

a8ci18n Jul 8, 2020

matticbot commented Jul 8, 2020

Addison-Stavlo left a comment

Addison-Stavlo Jul 8, 2020 •

edited

Loading

andrewserong commented Jul 9, 2020

andrewserong commented Jul 9, 2020

Addison-Stavlo commented Jul 9, 2020 •

edited

Loading

Addison-Stavlo commented Jul 9, 2020 •

edited

Loading

a8ci18n Jul 9, 2020

alshakero commented Jul 14, 2020 •

edited

Loading

a8ci18n Jul 14, 2020

Addison-Stavlo left a comment •

edited

Loading

Addison-Stavlo Jul 14, 2020 •

edited

Loading

alshakero Jul 15, 2020

alshakero commented Jul 15, 2020

ockham commented Aug 5, 2020

Copons commented Aug 24, 2020

Copons commented Aug 24, 2020 •

edited

Loading

Copons commented Aug 25, 2020

Addison-Stavlo commented Aug 26, 2020 •

edited

Loading

Copons commented Aug 26, 2020

Addison-Stavlo commented Aug 26, 2020

Copons commented Aug 27, 2020

Addison-Stavlo commented Aug 28, 2020

david-szabo97 commented Sep 2, 2020

Addison-Stavlo commented Sep 3, 2020

Addison-Stavlo left a comment

FSE: Add Current layout to starter page templates page layout picker when editing a homepage #43980

FSE: Add Current layout to starter page templates page layout picker when editing a homepage #43980

Conversation

andrewserong commented Jul 8, 2020 • edited by Copons Loading

Changes proposed in this Pull Request

Still to do

Screenshot

Testing instructions

matticbot commented Jul 8, 2020

matticbot commented Jul 8, 2020

a8ci18n Jul 8, 2020

Choose a reason for hiding this comment

matticbot commented Jul 8, 2020

Addison-Stavlo left a comment

Choose a reason for hiding this comment

Addison-Stavlo Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

andrewserong commented Jul 9, 2020

andrewserong commented Jul 9, 2020

Addison-Stavlo commented Jul 9, 2020 • edited Loading

Addison-Stavlo commented Jul 9, 2020 • edited Loading

a8ci18n Jul 9, 2020

Choose a reason for hiding this comment

alshakero commented Jul 14, 2020 • edited Loading

a8ci18n Jul 14, 2020

Choose a reason for hiding this comment

Addison-Stavlo left a comment • edited Loading

Choose a reason for hiding this comment

Addison-Stavlo Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

alshakero Jul 15, 2020

Choose a reason for hiding this comment

alshakero commented Jul 15, 2020

ockham commented Aug 5, 2020

Copons commented Aug 24, 2020

Copons commented Aug 24, 2020 • edited Loading

Copons commented Aug 25, 2020

Addison-Stavlo commented Aug 26, 2020 • edited Loading

Copons commented Aug 26, 2020

Addison-Stavlo commented Aug 26, 2020

Copons commented Aug 27, 2020

Addison-Stavlo commented Aug 28, 2020

david-szabo97 commented Sep 2, 2020

Addison-Stavlo commented Sep 3, 2020

Addison-Stavlo left a comment

Choose a reason for hiding this comment

andrewserong commented Jul 8, 2020 •

edited by Copons

Loading

Addison-Stavlo Jul 8, 2020 •

edited

Loading

Addison-Stavlo commented Jul 9, 2020 •

edited

Loading

Addison-Stavlo commented Jul 9, 2020 •

edited

Loading

alshakero commented Jul 14, 2020 •

edited

Loading

Addison-Stavlo left a comment •

edited

Loading

Addison-Stavlo Jul 14, 2020 •

edited

Loading

Copons commented Aug 24, 2020 •

edited

Loading

Addison-Stavlo commented Aug 26, 2020 •

edited

Loading