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

Block Library: Add features to the Post Excerpt block. #19715

Merged
merged 6 commits into from Feb 18, 2020

Conversation

epiqueras
Copy link
Contributor

@epiqueras epiqueras commented Jan 17, 2020

Closes #19698

Description

This PR implements the design from #19698.

How to test this?

Verify all the features from #19698 work as expected with a Post Excerpt block inserted in a regular post.

Screenshots

gif

Types of Changes

New Feature: The Post Excerpt block now has a more rich editing experience and allows you to customize the excerpt length and read more link directly.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

"category": "layout"
"category": "layout",
"attributes": {
"excerptLength": {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just "length"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's clearer that it refers to the excerpt part. The block also has the read more text and could evolve to have other things in the future.

Copy link
Member

Choose a reason for hiding this comment

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

This should probably be wordCount or wordLength.

Copy link
Contributor

Choose a reason for hiding this comment

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

wordCount sounds like the most accurate to me. Let's change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree because of this being a "max length" and not a word count but I don't think it's a big deal so I've updated it.

"moreText": {
"type": "string"
},
"showMoreOnNewLine": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an attribute or just something a theme can tweak with CSS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making it an attribute allows us to leverage different markup.

@paaljoachim
Copy link
Contributor

paaljoachim commented Jan 17, 2020

For instance adding a post excerpt block on the post itself would mean that this specific block would not show up on the frontend when viewing the post.
Will there be some kind of visual difference between the excerpt block and the rest of the post layout?

Meaning any block such as the post excerpt block that does not show up in the frontend post/page layout but has another specific feature could perhaps have a visual marker in the backend. To give it a clear difference between blocks used in the post/page layout and blocks that have a specific purpose not included in the regular layout.

@epiqueras
Copy link
Contributor Author

For instance adding a post excerpt block on the post itself would mean that this specific block would not show up on the frontend when viewing the post.

It does render the excerpt for that post.

In any case, limiting things like this is what settings.parent is for.

@karmatosed
Copy link
Member

Will there be some kind of visual difference between the excerpt block and the rest of the post layout?

I think this might be where CSS could be used. I could see exploring style variations but that feels like down to the theme almost.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Great work here :) One thing I think should be changed is the fact that usePostContentExcerpt has a custom logic that trims and cuts the post content in order to get the excerpt preview, while the server side render_block_core_post_excerpt uses the normal get_the_excerpt.

This will lead to things showing up in the editor which will not show up in the front.

As far as I can tell, I may be wrong, the REST API returns an element ['excerpt']['raw'] which is the result of some filters including wp_trim_excerpt. This filter/function takes care of generating an excerpt from the post data, so we shouldn't need to redo this logic. Also, the best thing it does, is to call strip_shortcodes and excerpt_remove_blocks which avoids bumping into issues like #19418

@epiqueras
Copy link
Contributor Author

This will lead to things showing up in the editor which will not show up in the front.

Do you have an example? They should be aligned.

This filter/function takes care of generating an excerpt from the post data, so we shouldn't need to redo this logic.

Yes, we do because we are dynamically changing the excerpt length in the client and we wish to preview it while we do so.

Also, the best thing it does, is to call strip_shortcodes and excerpt_remove_blocks which avoids bumping into issues like #19418

That's why I use rendered post content.

@draganescu
Copy link
Contributor

Do you have an example? They should be aligned.

Again, I am not 100% sure, but it appears that there are different sets of operations happening on each of the filters used for content or excerpt:

default-filters.php#L172

add_filter( 'the_content', 'do_blocks', 9 );
add_filter( 'the_content', 'wptexturize' );
add_filter( 'the_content', 'convert_smilies', 20 );
add_filter( 'the_content', 'wpautop' );
add_filter( 'the_content', 'shortcode_unautop' );
add_filter( 'the_content', 'prepend_attachment' );
add_filter( 'the_content', 'wp_make_content_images_responsive' );

add_filter( 'the_excerpt', 'wptexturize' );
add_filter( 'the_excerpt', 'convert_smilies' );
add_filter( 'the_excerpt', 'convert_chars' );
add_filter( 'the_excerpt', 'wpautop' );
add_filter( 'the_excerpt', 'shortcode_unautop' );
add_filter( 'get_the_excerpt', 'wp_trim_excerpt', 10, 2 );

Most notably the_excerpt does not do_blocks, instead it does excerpt_remove_blocks in wp_trim_excerpt. On the other hand the REST controller for posts applies the_content filter on $data['content']['rendered'].

@epiqueras
Copy link
Contributor Author

epiqueras commented Jan 27, 2020 via email

@draganescu
Copy link
Contributor

So I have tried to reproduce the bug in #19418 and it worked:

Screenshot 2020-01-27 at 19 37 02

The point in the image above is that the excerpt now contains a link, and it should in fact be empty. To reproduce this:

  1. add a post with an embed at the start
  2. in that post add more content
  3. use the excerpt block in that post or in single
  4. you will see by default the link of the embed, which should be stripped

@epiqueras
Copy link
Contributor Author

I think the correct fix here is not rendering that URL as visible text like that.

@epiqueras
Copy link
Contributor Author

What's blocking this?

const [ excerpt, setExcerpt ] = useEntityProp( 'postType', 'post', 'excerpt' );
return <PlainText value={ excerpt } onChange={ setExcerpt } />;
const postContentExcerpt = usePostContentExcerpt( excerptLength );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because the backend defaults to the post content if the excerpt is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and you can't control the max length dynamically.


function PostExcerptDisplay() {
function usePostContentExcerpt( excerptLength ) {
const [ , , { raw: rawPostContent } ] = useEntityProp(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have three returned values in the array, is the first one normalized or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first one is the edited value, yes.

keepPlaceholderOnFocus
/>
{ ! showMoreOnNewLine && ' ' }
<RichText
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be inside a paragraph if "showMoreOnNewLine" is true (to match the backend)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updated.

@epiqueras epiqueras force-pushed the add/post-excerpt-block-features branch from 87dc653 to 2e35754 Compare February 18, 2020 13:32
@@ -0,0 +1,2 @@
<!-- wp:post-title /-->
<!-- wp:post-content /-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is an unrelated change, do we want it or should we remove it from the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, deleted.

@epiqueras epiqueras force-pushed the add/post-excerpt-block-features branch from 3502985 to 307facc Compare February 18, 2020 15:23
@epiqueras epiqueras force-pushed the add/post-excerpt-block-features branch from 307facc to ece6591 Compare February 18, 2020 15:31
@epiqueras epiqueras merged commit 3d3c3e1 into master Feb 18, 2020
@epiqueras epiqueras added this to Done in Phase 2 via automation Feb 18, 2020
@epiqueras epiqueras deleted the add/post-excerpt-block-features branch February 18, 2020 20:56
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.6 Feb 26, 2020
attributes,
setAttributes,
isSelected,
} ) {
if ( ! useEntityId( 'postType', 'post' ) ) {
return 'Post Excerpt Placeholder';

Choose a reason for hiding this comment

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

This text cannot be translated if it's defined statically.

$filter_excerpt_length
);

$output = '<p>' . get_the_excerpt( $post );

Choose a reason for hiding this comment

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

This element should have a class name, so that it can be targeted by Theme and Plugin developers.

@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
No open projects
Phase 2
  
Done
Development

Successfully merging this pull request may close these issues.

New Block: Post Excerpt
8 participants