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 a Post Comments block. #19581

Merged
merged 6 commits into from
Mar 18, 2020
Merged

Conversation

epiqueras
Copy link
Contributor

Description

This PR adds a new Post Comments block akin to the Post Title and Post Content blocks.

How has this been tested?

  • Inserted Post Comments block in a post.
  • Confirmed post comments rendered in the editor and front end.
  • Inserted Post Comments block in a template.
  • Confirmed post comments placeholder rendered in the editor and the relevant post comments rendered in the front end.

Types of Changes

New Feature: There is a new Post Comments block for template building.

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.

@youknowriad
Copy link
Contributor

Added "design feedback" tag for designers to start thinking about the potential of these blocks (style variations, styling...)

packages/block-library/src/post-comments/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/post-comments/edit.js Outdated Show resolved Hide resolved
}

export default function PostCommentsEdit() {
const postId = useEntityId( 'postType', 'post' );
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jan 17, 2020

Choose a reason for hiding this comment

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

We may have other post types that support comments, for all these post types we are rendering 'Post Comments Placeholder' while I think we should render the same we render for posts (the comments in that post).

For post types that don't support comments but are not templates or template parts, I think we should not show the block on the inserter e.g: remove it from the allowed blocks or if we show in the inserter we should say in the block that comments are not supported. The block should handle the condition and show this message even if hidden from the inserter because a CPT may have supported comments in the past and then the user may disable the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, but we'll have to handle it at another level in the code.

It will likely require changes to core-data to get post type capabilities and have CPTs act as posts under certain conditions.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a very good remark and one that should lead to some rethinking in how the context EntityProvider and useEntityProp/useEntityId works.

Ideally, these hooks should just work no matter the CPT (all CPTs can have all properties) and should not have "post" hard-coded.

My first thinking here is that we should be able to do something like:

const entityId = useEntityId( 'postType' );
const entityContent = useEntityProp( 'postType', null, 'content' );
``
`

And these should basically return the first entity in the tree with the "kind" (postType)

We could even imagine a use-case for a post where we'd want any entity (less likely but why not)

const entityId = useEntityId();
const entityContent = useEntityProp( null, null, 'content' )


Too bad `useEntityProp` is a stable API now though cause "null" for  the first and second args is not great.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the first block where it's specific to "post" so we need to solve this ASAP on a follow-up 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.

Yes, this is what I suggested.

But, #19685 (comment) is my preferred alternative that also works on the server.

@epiqueras
Copy link
Contributor Author

Can we merge this scaffold to avoid more merge conflicts in the shared files?

@jorgefilipecosta

@kjellr kjellr added the New Block Suggestion for a new block label Feb 18, 2020
@ockham
Copy link
Contributor

ockham commented Feb 24, 2020

Rebased.

@github-actions
Copy link

github-actions bot commented Feb 24, 2020

Size Change: +148 B (0%)

Total Size: 857 kB

Filename Size Change
build/block-library/index.js 111 kB +127 B (0%)
build/core-data/index.js 10.6 kB +21 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 100 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/style-rtl.css 7.42 kB 0 B
build/block-library/style.css 7.43 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.2 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.2 kB 0 B
build/edit-post/style-rtl.css 8.52 kB 0 B
build/edit-post/style.css 8.51 kB 0 B
build/edit-site/index.js 5.07 kB 0 B
build/edit-site/style-rtl.css 2.53 kB 0 B
build/edit-site/style.css 2.53 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 44 kB 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.58 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Left a few minor notes, but looking good overall 👍

@youknowriad youknowriad mentioned this pull request Mar 11, 2020
53 tasks
@noahtallen
Copy link
Member

Rebased and added an entity label for Comments.

@noahtallen
Copy link
Member

I added a "no comments" placeholder, as well as several TODO notes based on the feedback in the PR. I think the scaffolding for this PR is ready to go now 👍

@epiqueras epiqueras requested a review from ntwb as a code owner March 18, 2020 00:34
@youknowriad youknowriad merged commit 6ddcdc3 into master Mar 18, 2020
@youknowriad youknowriad deleted the add/post-comments-block branch March 18, 2020 08:27
@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. New Block Suggestion for a new block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants