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

Latest Post Block (Post Content Support) iteration #14627

Merged

Conversation

AmartyaU
Copy link
Contributor

@AmartyaU AmartyaU commented Mar 26, 2019

Description

Adds Post content support in #1594. @ajitbohra is working on all the other parts of the issue (#13698).

Work In Progress

Screenshots

Screen Shot 2019-03-30 at 10 51 01 PM

Editor Side Display Excerpt

Screen Shot 2019-03-30 at 10 45 33 PM

Rendered Display Excerpt

Screen Shot 2019-03-30 at 10 45 53 PM

Editor Side Display Full Post

Screen Shot 2019-03-30 at 10 46 25 PM

Rendered Display Full Post

Screen Shot 2019-03-30 at 10 46 46 PM

@gziolo @melchoyce You can track the progress on the issue here!

@gziolo
Copy link
Member

gziolo commented Mar 26, 2019

I have not modified index.php because I am not sure if we are even using it, because I believe right now we are not using ServerSideRender to get the result from the index.php file and instead, we are just using the result from the latestPostsQuery to display everything on the frontend

Try to see the preview of the post with new options applied :) ServerSideRender has its own pros and cons. The benefit of using it is that you don't have to implement the same logic twice in both JavaScript and PHP, but the downside is that every time you change something to see the result you need to do network request. Latest Posts block is different as it has fully JS based edit implementation with immediate re-render after any change but the downside is that you need to re-implement PHP version as well.

@gziolo gziolo added [Block] Latest Posts Affects the Latest Posts Block [Type] Enhancement A suggestion for improvement. labels Mar 26, 2019
@@ -4753,8 +4753,7 @@
"ansi-regex": {
"version": "2.1.1",
"bundled": true,
"dev": true,
"optional": true
"dev": true
Copy link
Member

Choose a reason for hiding this comment

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

That's surprising. Ideally, there should be no difference in package-lock.json file. Which version of npm do you use?

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 am using 6.4.1

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove changes to package-lock.json from this PR? You can copy the original lock file from master branch.

import {
InspectorControls,
BlockAlignmentToolbar,
BlockControls,
} from '@wordpress/block-editor';
} from '@wordpress/editor';
Copy link
Member

Choose a reason for hiding this comment

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

There are some ongoing code reorganizations and those components were moved to the new package. They will continue to work from @wordpress/editor (with maybe some warnings) but it should stay unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up! Should I change it back to @wordpress/block-editor too?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@AmartyaU
Copy link
Contributor Author

I have completed most of the functionality, but I have a few design and development questions.

Design Questions:

  1. In the latest post block, do we want to show the post the latest post block is in currently?

I think this is redundant and may take up unnecessary space in 'Display Full Post' but may be useful in 'Display Post Excerpt', because it won't be repeating the same information already on the page in the latter case.

  1. At the moment, if a post doesn't have an excerpt, I am not not displaying anything. Should I display the post content instead in this case?

I would prefer not displaying anything as an excerpt is inherently a summary and the first few lines of the post content wouldn't fit in in this category generally.

  1. In the 'Display Full Post', both on the editor and render side, I am displaying everything in a particular post except dynamic blocks (like RSS, latest post block). Is this fine?

Development Question:

  1. In the PHP file, I am not able to escape HTML for the post_content before rendering it, because I need the display the full post HTML content with identical formatting. Is this safe? What would be a good workaround to ensure security with displaying HTML?

@melchoyce
Copy link
Contributor

In the latest post block, do we want to show the post the latest post block is in currently?

My gut says "no," but I'm open to hearing other opinions.

At the moment, if a post doesn't have an excerpt, I am not not displaying anything. Should I display the post content instead in this case?

We should show the post content truncated to whatever number of characters is being used for excerpts.

In the 'Display Full Post', both on the editor and render side, I am displaying everything in a particular post except dynamic blocks (like RSS, latest post block). Is this fine?

Can you add some screenshots? Hard to visualize.

Thanks for all your work on this!

@AmartyaU
Copy link
Contributor Author

AmartyaU commented Apr 2, 2019

We should show the post content truncated to whatever number of characters is being used for excerpts.

Just updated the code to display truncated post content when excerpt is not present!

Can you add some screenshots? Hard to visualize.

For example, I added a RSS feed to the 'Bulleted Post' post
Screen Shot 2019-04-02 at 11 27 24 AM

But this RSS feed won't be displayed on either the editor or rendered side, because it's a dynamic block I am assuming

Screen Shot 2019-04-02 at 11 27 38 AM

Screen Shot 2019-04-02 at 11 27 46 AM

@getdave
Copy link
Contributor

getdave commented Apr 3, 2019

Development Question:

  1. In the PHP file, I am not able to escape HTML for the post_content before rendering it, because I need the display the full post HTML content with identical formatting. Is this safe? What would be a good workaround to ensure security with displaying HTML?

I think you might want wp_kses_post.

Testing

It's great to see this coming on so well. Great work! 👍 I've tested and have noticed a few issues which I'll document below as I find them:

  • Number of Posts UI doesn't reflect the default number of posts shown
    Screen Shot 2019-04-03 at 10 25 14

  • Number of Posts UI - the slider UI itself doesn't update. However, dragging it left/right does adjust the number of posts displayed.
    Screen Capture on 2019-04-03 at 10-27-21

  • Excerpt truncation - should there be a Read more link here? The docs say:

When using the excerpt feature WordPress does not automatically provide a link to a page containing the full post

It then goes on to suggest how to add one manually. I think we should have an option to add/remove a Read more link?

Screen Shot 2019-04-03 at 10 30 34

  • Author meta - unless @melchoyce disagrees, I feel this should be styled so as to be visually consistent with the date in the meta.

Screen Shot 2019-04-03 at 10 34 31

  • Sticky posts - currently there is no option to respect Sticky Posts. This may well be intentional but it feels like it could be a useful feature. @melchoyce what say you?

Screen Shot 2019-04-03 at 10 38 45

  • Post content + grid layout - at the moment it is possible to display the full rendered content of the post (ie: the_content()) whilst in grid layout mode. This can result in some pretty horrible layouts, especially as the numebr of columns increases. I suspect this would be worse should the post content contain more complex Blocks. Should we protect people from themselves and disable grid mode if the full post content is to be displayed? If so we'd need a warning message below the toggle option.

Screen Shot 2019-04-03 at 10 40 52

  • Default list styling in list layout mode - I've noticed the editor shows the default list bullet points when in list mode. It might make the Block look neater if these bullets were removed and the list "reset".

Screen Shot 2019-04-03 at 10 48 27

@getdave
Copy link
Contributor

getdave commented Apr 3, 2019

@youknowriad Technical question. I can imagine developers needing to amend the markup generated by this Block. For example you might need the author to be displayed underneath the post content rather than above it. Would the blocks.getSaveElement filter be suitable for this purpose? Do you have any better suggestions?

I'm conscious that if a theme has a particular loop() setup, then the Theme author is going to need to write it once for the Theme PHP and again for this Block.

@draganescu
Copy link
Contributor

draganescu commented Apr 22, 2019

Hi @AmartyaU great job! The column spacing is fixed! :D

However, I found some new issues :P

1. The read more link is floated when the post contains a gallery:

Screenshot 2019-04-22 at 17 44 37

This causes even worse problems when the gallery is in the middle column:

Screenshot 2019-04-22 at 17 45 13

2. We should strip the tags and formatting of the excerpt so that we can be sure of the result, not like in this case the bullets ruin the layout:

Screenshot 2019-04-22 at 17 47 05

I think we shouldn't display galleries at all, and strip tags would help in this problem too as all img tags will be removed.

@AmartyaU
Copy link
Contributor Author

I think we shouldn't display galleries at all, and strip tags would help in this problem too as all img tags will be removed.

I think that's the right thing to do! So basically I should strip all the tags on the editor side so that when post content is displayed in place of excerpt (when a post doesn't have an excerpt), it follows a proper format (without images)? I think server side already does this, so I can just replicate that behavior.

@AmartyaU AmartyaU force-pushed the update/latest-post-block_post-content branch from 3c66c1e to 2699ea4 Compare April 24, 2019 01:58
@AmartyaU
Copy link
Contributor Author

AmartyaU commented Apr 24, 2019

I just added a commit that proposes a solution to the above, but I would appreciate it if you would take a look at it. Instead of using Regex, I used https://ctrlq.org/code/19813-convert-html-to-text in my solution and it looks to be working fine. The excerpt displayed on the editor is identical to that displayed on the server side as shown below:

Full Post Content

Screen Shot 2019-04-23 at 9 36 47 PM

Excerpt on Editor Side

Screen Shot 2019-04-23 at 10 00 03 PM

Excerpt on Server Side

Screen Shot 2019-04-23 at 9 37 18 PM

@getdave
Copy link
Contributor

getdave commented Apr 29, 2019

Great work on this.

So basically I should strip all the tags on the editor side so that when post content is displayed in place of excerpt (when a post doesn't have an excerpt), it follows a proper format (without images)?

I'm concerned about this. Are we proposing stipping all blocks/tags from the "Full Content" version?

I can envisage a use case where someone wants to display their full latest posts on a page. If all tags and blocks are stripped from the content then that use case will not work.

It's far from ideal that the layout looks poor if this block is used within another Block or the post themselves contain complex Blocks. However, that seems like more of an edge case to me.

What about adding an option to strip tags/content rather than applying as a blanket default?

@AmartyaU
Copy link
Contributor Author

I'm concerned about this. Are we proposing stipping all blocks/tags from the "Full Content" version?

No, we are not proposing to strip all blocks/tags from the "Full Content" version. The "Full Content" version will still display everything as before. We are just stripping the blocks/tags for the excerpt in the case when a post doesn't have an excerpt and has to use its post content. Since excerpts don't have formatting, we want to be consistent when a post uses its post content as its excerpt and that is the reason we are stripping all tags in that particular case!

@draganescu
Copy link
Contributor

@getdave indeed, the tags are stripped only from the excerpt, especially the one that is generated automagically from the post. Do you find any reason to allow them? The problem with tags in automatic excerpts is that you can mistakenly print unclosed tags and also some formatting or content (media) doesn't make any sense in an excerpt.

@AmartyaU great work! I have left only one comment. other than that we're good to go!

@getdave
Copy link
Contributor

getdave commented Apr 30, 2019

@AmartyaU One more thing I noticed is that when articles are displayed along with post content the markup isn't particularly accessible.

Screen Shot 2019-04-30 at 13 54 18

In this context my questions are:

  1. Should the article be wrapped in an <article> tag as it is now a full piece of content and not just a title or title + excerpt. If feels like syndicatable content in its own right.
  2. Given the above, should the title of the post be wrapped in a valid heading level tag? You will see from the screenshot above that if the article content contains headings (which it likely will) then the markup of the Block becomes quite confusing. It might be less so if it were within an article.

These may be problems to be solved in a v2. If so I'm happy to raise a separate issue.

@AmartyaU
Copy link
Contributor Author

AmartyaU commented May 1, 2019

These may be problems to be solved in a v2. If so I'm happy to raise a separate issue.

I think raising a separate issue about this would be best!

@youknowriad
Copy link
Contributor

Nice addition here. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Latest Posts Affects the Latest Posts Block Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants