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.

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

FSE: Toggle editor post title visibility with a filter / Set block template editor setting #34296

Merged
merged 3 commits into from
Jun 27, 2019

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Jun 26, 2019

Changes proposed in this Pull Request

Toggle editor post title visibility with a filter

This PRs implements a more robust approach than #34214 as suggested by @gwwar in order to avoid potential rendering flashes (#34214 (comment)).

Now, the a8c/post-content block always renders a post title before the content, but is hidden by default. It will show up when the admin_body_class filter adds the show-post-title-before-content class, which will also hide the regular editor post title. This makes unnecessary the selector isFullSitePage added in #34214, so it has been removed on this PR.

Set block template editor setting

The above caused a side effect. Since the FSE plugin creates a default template that is assigned to all pages without a specific template, we would get the show-post-title-before-content class when creating a new page. This was making the post title to be missing because at the moment we are not applying the template parts when we start a new post.

This has been fixed on this PR by defining a block template on the template editor setting

Follow-up

This PR will introduce a couple of known issues:

  • Seems that the page templates (Portfolio, Services, etc) assume the page will be empty. So now that we apply the FSE template when starting a new page, selecting any other template than Blank will result in the content of the selected page template to be appended after the footer template part.
  • The a8c/post-content block can be removed by the user, resulting in a missing post title since the show-post-title-before-content would keep the regular editor post title hidden.

Should we fix them before landing this PR?

Testing instructions

  • Apply this branch to your FSE testing environment.
  • Trash all templates and template parts.
  • Deactivate and then reactivate the FSE plugin.
  • Create a new page.
  • Select the Blank template.
  • Make sure the default FSE template is applied.
  • Make sure the title of the page is displayed below the header template part and at the beginning of the post content block.
  • Try inserting a post content blog a not full site page (i.e. in a regular post or in template).
  • Verify the post title is visible in the editor and the default FSE template is not applied.

@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Full Site Editing labels Jun 26, 2019
@mmtr mmtr requested review from a team as code owners June 26, 2019 06:18
@mmtr mmtr self-assigned this Jun 26, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

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.

@mmtr mmtr changed the title Update/fse set template editor setting FSE: Toggle editor post title visibility with a filter / Set block template editor setting Jun 26, 2019
@kwight
Copy link
Contributor

kwight commented Jun 26, 2019

Everything works as described (including the issues mentioned). I'd say we merge, then address the issues as individual followups.

@mmtr mmtr merged commit 0fc56bc into master Jun 27, 2019
@mmtr mmtr deleted the update/fse-set-template-editor-setting branch June 27, 2019 01:22
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 27, 2019
@vindl
Copy link
Member

vindl commented Jul 5, 2019

It looks like this PR broke the New Page functionality - when trying to insert a new page after it I get a blank screen with the following error:

Uncaught (in promise) TypeError: Cannot read property 'attributes' of undefined

Created an issue to track it here #34481.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants