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

Templates: Assign default post format block as template setting #9287

Merged
merged 1 commit into from
Aug 29, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Aug 23, 2018

This pull request seeks to refactor default block by post format to be effected as a template.

This is being proposed for two primary reasons:

  • It is a simplification, reusing existing template mechanisms in place of an additional step for enacting the default post format block
  • As an extension of the previous point, it will serve to benefit a planned subsequent refactoring of editor initialization aimed at eliminating "setup" steps of the editor, for which the default post format block is otherwise difficult to mimic in state without a designated start point.

There are two notable differences from how this behaves in master:

  • The template is not enacted except when editing a new post.
    • Arguably, I find this behavior, both for the default post format block and templates in general, to be buggy. For example, on master try:
      1. Set default post format to Image on Settings > Writing
      2. Navigate to Posts > Add New
      3. Add a title
      4. Remove the default image block
      5. Save the post
      6. Reload the editor
        • Expected: My changes are reflected (i.e. the removal of the default block)
        • Actual: The image placeholder block returns! Again, I acknowledge this could be an argued as intentional, but to me it seems like the default block should serve as a starting point niceity, but ultimately we respect the user's decisions to manipulate or remove those defaults.
  • The default block is no longer inserted when changing Post Format field
    • Another niceity, but I don't find it to be a worrisome change

Testing instructions:

Verify there are no regressions in the behavior of the default block by post format.

Ensure new end-to-end tests pass:

npm run test-e2e

@aduth aduth added the [Feature] Templates API Related to API powering block template functionality in the Site Editor label Aug 23, 2018
@aduth aduth requested a review from jorgefilipecosta August 23, 2018 18:52
await newPost( { postType: 'book' } );
} );
describe( 'templates', () => {
describe( 'Using a CPT with a predefined template', () => {
Copy link
Member Author

@aduth aduth Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these existing "Using a CPT" test cases have been changed. They're simply grouped / nested within a new "templates" describe block.

Edit: Scratch that, while I was here, I normalized to use getEditedPostContent instead of the existing "switch to code editor" logic.

@aduth aduth force-pushed the update/default-post-format-block branch 2 times, most recently from 0b0b5b7 to 109d0ca Compare August 24, 2018 14:34
@aduth aduth requested a review from a team August 27, 2018 13:18
@aduth aduth added this to the 3.7 milestone Aug 27, 2018
@@ -228,6 +229,11 @@ export function getDefaultBlockName() {
* @return {string} Block name.
*/
export function getDefaultBlockForPostFormat( postFormat ) {
deprecated( 'getDefaultBlockForPostFormat', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should deprecate this selector as it may be a useful selector for plugins, that may want to have a UI based on post formats. We also have a possible use case of this selector, the getSuggestedPostFormat selector. Currently is not using this selector but I think we may do a refactor there to use it instead of removing the selector.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second thought, a refactor of getSuggestedPostFormat may not be possible as in this selector we may have multiple blocks mapping to a single post format.
I still think getDefaultBlockForPostFormat may be useful for plugins but If we don't have a use case for it we should not be sending its bytes so probably what we have now is the best option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to optimize toward avoiding keeping things around because we'd anticipate them to possibly be useful. In this particular case, it's also a heavy maintenance burden to maintain mapping between post formats and default block both within JavaScript and in PHP. If it's something we want in the future, I think it'd be better for us to refactor how we provide it, bootstrapping from some constant (optionally filtered) defined server-side.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some tests on the post formats (post init and suggestions) and the tests went well, everything seems to be working correctly. I think removing this system of starting a post with a block already inserted and using the one from templates is a positive change 👍

@aduth aduth force-pushed the update/default-post-format-block branch from 109d0ca to 45ef23b Compare August 29, 2018 20:52
@aduth aduth merged commit 540778b into master Aug 29, 2018
@aduth aduth deleted the update/default-post-format-block branch August 29, 2018 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants