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

Fix: Follow discussion settings in the comments block edit #44463

Merged
merged 6 commits into from
Oct 5, 2022

Conversation

SantosGuillamot
Copy link
Contributor

What?

This Pull Request aims to respect all the discussion settings in the new Comments block while working in the Site Editor. We are loading some placeholders, and they should follow the discussion settings as much as possible. We decided to show 3 comments (to not overload it with too many comments), but in some cases it wasn't working properly.

Bear in mind that, while using the Comments block in the Post Editor, the use case is different. Inside the Site Editor we are using placeholders, but in the Post Editor it gets real data from the REST API, which is a bit trickier. Right now, we are only fetching the top-level comments (due to this argument) and the _embedded children property only includes the next-level children. So we aren’t fetching more than 2 levels at this moment. If we remove the parent: 0 argument, we would get all the comments on the post, but it gets tricky as well. We would have to add some logic to nest the comments and there could be a case where the children are fetched and not the parent, which could cause weird UX issues. Imagine a case where the per_page is set to 10 and we have a parent comment with 15 replies. Only the latest 10 replies would be fetched and the parent would be excluded.

This part is not covered in this Pull Request anyway.

Why?

Depending on the discussion settings, the UX right now is a bit weird, so I suggest a new way of handling it to prevent this. For example, at this moment, if the "Enable threaded (nested) comments X levels deep" setting is disabled, just one comment is shown (this code).

Screenshot 2022-09-26 at 12 58 40

How?

I refactored the code a bit and changed slightly the UX. With this I aimed to provide the following workflows:

  1. Don't show only two comments if the break comments into pages is disabled (even if it is set to 2):

Screenshot 2022-09-26 at 14 18 06

For this, we have to read the pageComments setting.

  1. Show at least three top-level comments unless per_page is less than three and it is enabled.

Screenshot 2022-09-26 at 14 19 52

  1. Show all the nested levels in the first comment if the setting is enabled.

Screenshot 2022-09-26 at 14 20 48

In this case, I wasn't sure if we should limit it to three levels. The max for this option is 10, and I thought it could make sense to show the user how it would look like if the levels is too high.

Testing Instructions

  1. Open the Site Editor with a block-based theme using the new Comments block (the Twenty Twenty-Three theme for example).
  2. Navigate to the Single template, where the Comments should be placed.
  3. Check the placeholders with the different Discussion Settings.

@SantosGuillamot SantosGuillamot added [Type] Bug An existing feature does not function as intended [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop labels Sep 26, 2022
@github-actions
Copy link

github-actions bot commented Sep 26, 2022

Size Change: +4.87 kB (0%)

Total Size: 1.27 MB

Filename Size Change
build/block-editor/index.min.js 166 kB +67 B (0%)
build/block-editor/style-rtl.css 15.4 kB -26 B (0%)
build/block-editor/style.css 15.3 kB -28 B (0%)
build/block-library/blocks/code/style-rtl.css 121 B +18 B (+17%) ⚠️
build/block-library/blocks/code/style.css 121 B +18 B (+17%) ⚠️
build/block-library/blocks/navigation-submenu/view.min.js 0 B -423 B (removed) 🏆
build/block-library/blocks/navigation/editor-rtl.css 2.01 kB +8 B (0%)
build/block-library/blocks/navigation/editor.css 2.02 kB +8 B (0%)
build/block-library/blocks/navigation/style-rtl.css 2.17 kB +2 B (0%)
build/block-library/blocks/navigation/style.css 2.16 kB +5 B (0%)
build/block-library/blocks/paragraph/style-rtl.css 279 B +19 B (+7%) 🔍
build/block-library/blocks/paragraph/style.css 281 B +21 B (+8%) 🔍
build/block-library/blocks/post-featured-image/editor-rtl.css 586 B +39 B (+7%) 🔍
build/block-library/blocks/post-featured-image/editor.css 584 B +39 B (+7%) 🔍
build/block-library/blocks/post-terms/style-rtl.css 96 B +23 B (+32%) 🚨
build/block-library/blocks/post-terms/style.css 96 B +23 B (+32%) 🚨
build/block-library/blocks/site-title/editor-rtl.css 116 B +32 B (+38%) 🚨
build/block-library/blocks/site-title/editor.css 116 B +32 B (+38%) 🚨
build/block-library/blocks/video/editor-rtl.css 691 B +130 B (+23%) 🚨
build/block-library/blocks/video/editor.css 694 B +131 B (+23%) 🚨
build/block-library/editor-rtl.css 11.2 kB +101 B (+1%)
build/block-library/editor.css 11.2 kB +101 B (+1%)
build/block-library/index.min.js 192 kB +411 B (0%)
build/block-library/style-rtl.css 12.3 kB +44 B (0%)
build/block-library/style.css 12.3 kB +47 B (0%)
build/block-serialization-default-parser/index.min.js 1.12 kB +18 B (+2%)
build/blocks/index.min.js 49.8 kB +49 B (0%)
build/components/index.min.js 202 kB +3.42 kB (+2%)
build/components/style-rtl.css 11.2 kB -248 B (-2%)
build/components/style.css 11.2 kB -249 B (-2%)
build/compose/index.min.js 12.5 kB +41 B (0%)
build/edit-post/index.min.js 31.1 kB +276 B (+1%)
build/edit-post/style-rtl.css 6.97 kB +31 B (0%)
build/edit-post/style.css 6.97 kB +31 B (0%)
build/edit-site/index.min.js 57.8 kB +220 B (0%)
build/edit-site/style-rtl.css 8.36 kB +6 B (0%)
build/edit-site/style.css 8.35 kB +5 B (0%)
build/list-reusable-blocks/index.min.js 2.13 kB +389 B (+22%) 🚨
build/style-engine/index.min.js 1.46 kB +5 B (0%)
build/widgets/index.min.js 7.21 kB +34 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 982 B
build/annotations/index.min.js 2.76 kB
build/api-fetch/index.min.js 2.26 kB
build/autop/index.min.js 2.14 kB
build/blob/index.min.js 475 B
build/block-directory/index.min.js 7.09 kB
build/block-directory/style-rtl.css 990 B
build/block-directory/style.css 991 B
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 84 B
build/block-library/blocks/avatar/style.css 84 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 482 B
build/block-library/blocks/button/editor.css 482 B
build/block-library/blocks/button/style-rtl.css 523 B
build/block-library/blocks/button/style.css 523 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 100 B
build/block-library/blocks/categories/style.css 100 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 187 B
build/block-library/blocks/comment-template/style.css 185 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 834 B
build/block-library/blocks/comments/editor.css 832 B
build/block-library/blocks/comments/style-rtl.css 632 B
build/block-library/blocks/comments/style.css 630 B
build/block-library/blocks/cover/editor-rtl.css 605 B
build/block-library/blocks/cover/editor.css 607 B
build/block-library/blocks/cover/style-rtl.css 1.57 kB
build/block-library/blocks/cover/style.css 1.55 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 346 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 948 B
build/block-library/blocks/gallery/editor.css 950 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 384 B
build/block-library/blocks/group/editor.css 384 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 327 B
build/block-library/blocks/html/editor.css 329 B
build/block-library/blocks/image/editor-rtl.css 884 B
build/block-library/blocks/image/editor.css 882 B
build/block-library/blocks/image/style-rtl.css 627 B
build/block-library/blocks/image/style.css 630 B
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 463 B
build/block-library/blocks/latest-posts/style.css 462 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 705 B
build/block-library/blocks/navigation-link/editor.css 703 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 443 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 363 B
build/block-library/blocks/page-list/editor.css 363 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 317 B
build/block-library/blocks/paragraph/editor.css 317 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 493 B
build/block-library/blocks/post-comments-form/style.css 493 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/style-rtl.css 315 B
build/block-library/blocks/post-featured-image/style.css 315 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 282 B
build/block-library/blocks/post-template/style.css 282 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 326 B
build/block-library/blocks/pullquote/style.css 325 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 282 B
build/block-library/blocks/query-pagination/style.css 278 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 439 B
build/block-library/blocks/query/editor.css 439 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 409 B
build/block-library/blocks/search/style.css 406 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 464 B
build/block-library/blocks/shortcode/editor.css 464 B
build/block-library/blocks/site-logo/editor-rtl.css 488 B
build/block-library/blocks/site-logo/editor.css 488 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 322 B
build/block-library/blocks/spacer/editor.css 322 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 494 B
build/block-library/blocks/table/editor.css 494 B
build/block-library/blocks/table/style-rtl.css 611 B
build/block-library/blocks/table/style.css 609 B
build/block-library/blocks/table/theme-rtl.css 190 B
build/block-library/blocks/table/theme.css 190 B
build/block-library/blocks/tag-cloud/style-rtl.css 239 B
build/block-library/blocks/tag-cloud/style.css 239 B
build/block-library/blocks/template-part/editor-rtl.css 235 B
build/block-library/blocks/template-part/editor.css 235 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/style-rtl.css 174 B
build/block-library/blocks/video/style.css 174 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 162 B
build/block-library/classic.css 162 B
build/block-library/common-rtl.css 1.02 kB
build/block-library/common.css 1.02 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/theme-rtl.css 719 B
build/block-library/theme.css 722 B
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/core-data/index.min.js 15.5 kB
build/customize-widgets/index.min.js 11.3 kB
build/customize-widgets/style-rtl.css 1.38 kB
build/customize-widgets/style.css 1.38 kB
build/data-controls/index.min.js 653 B
build/data/index.min.js 8.08 kB
build/date/index.min.js 32.1 kB
build/deprecated/index.min.js 507 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.7 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.99 kB
build/edit-navigation/style.css 4 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.34 kB
build/edit-widgets/style.css 4.34 kB
build/editor/index.min.js 41.6 kB
build/editor/style-rtl.css 3.62 kB
build/editor/style.css 3.61 kB
build/element/index.min.js 4.68 kB
build/escape-html/index.min.js 537 B
build/experiments/index.min.js 868 B
build/format-library/index.min.js 6.77 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.64 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.77 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.78 kB
build/keycodes/index.min.js 1.83 kB
build/list-reusable-blocks/style-rtl.css 835 B
build/list-reusable-blocks/style.css 835 B
build/media-utils/index.min.js 2.93 kB
build/notices/index.min.js 963 B
build/nux/index.min.js 2.06 kB
build/nux/style-rtl.css 732 B
build/nux/style.css 728 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.22 kB
build/preferences/index.min.js 1.3 kB
build/primitives/index.min.js 933 B
build/priority-queue/index.min.js 1.58 kB
build/react-i18n/index.min.js 696 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.74 kB
build/reusable-blocks/index.min.js 2.21 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.6 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.53 kB
build/token-list/index.min.js 644 B
build/url/index.min.js 3.61 kB
build/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 268 B
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.19 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@ockham ockham added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 26, 2022
@ockham
Copy link
Contributor

ockham commented Sep 26, 2022

Thanks a lot for working on this @SantosGuillamot!

I've tried this with my "threaded comments" settings unchanged (which previously got me 3 nested placeholder comments in the Site Editor).

This is now up to 7. IMO, that's too much from a UX point-of-view (and it takes up too much space); after all, they're just placeholders to exemplify a concept 😅 (Also, the heading still states "3 comments" -- it should probably reflect the real number.)

Before After
image image

@SantosGuillamot
Copy link
Contributor Author

SantosGuillamot commented Sep 27, 2022

This is now up to 7. IMO, that's too much from a UX point-of-view.

Yeah, I thought about that too but I wasn't sure. I have just limited it to two nested comments. Anyway, I believe it still makes sense to keep three top-level comments to show the full experience. After all, it is how the posts will look like.

Screenshot 2022-09-27 at 09 01 22

the heading still states "3 comments" -- it should probably reflect the real number.

You're right, although I'm not sure how this should be handled. The first thing that comes to my mind is to read here the perPage, pageComments, threadComments and threadCommentsDepth settings as we are doing in the Comment Template placeholder. And add similar logic there. But I'm not sure it is the best approach. Any ideas?

EDIT: I have just pushed a commit to show what I mean. It works, but we have to keep it synced with the logic changes made to the Comment Template placeholder.

@ockham
Copy link
Contributor

ockham commented Sep 27, 2022

Thanks Mario, this is much better!

There's a few minor tweaks I'd still like us to make:

  1. TBH, 5 comments are still too much IMO; they take up too much space without providing that much information to users. Let's keep it to 3 or 4.
  2. This also applies to the level of nesting: Let's always cut off after 3 levels of nested comments. (I think it's enough to illustrate the idea to users. Just because they have threading set to a large number like 8 doesn't mean we need to show all eight levels in the placeholder.)

TBH, I think we can just leave it at 3. This allows us to hard-wire the Comments Title placeholder back to "3", and gives us the following configurations if I'm not mistaken:

Threaded (>=3):

  1. Comment
    i. Comment
    • Comment

Threaded (==2):

  1. Comment
    i. Comment
  2. Comment

Unthreaded:

  1. Comment
  2. Comment
  3. Comment

I know it doesn't reflect each and every possibility, but I don't think that has to be the goal for a placeholder.

Curious to hear your thoughts!

@SantosGuillamot
Copy link
Contributor Author

Thanks for the feedback 🙂

Right now, the maximum number of comments is 5, including nested comments. I created a limit at 3 nested comments so no more levels should show. But I was still adding 2 top-level comments if the pagination setting allows it.

Anyways, if it feels there are still too many I can remove the other top-level comments as you suggest to always show 3 comments at max.

This allows us to hard-wire the Comments Title placeholder back to "3"

Not sure about that. Imagine you have the pagination setting set to 2 comments per page and no nested comments, it would have 2 replies for example.

To be honest, I don't have a strong opinion in this regard, so we can go your way.

@ockham
Copy link
Contributor

ockham commented Sep 27, 2022

Thanks for the feedback 🙂

Right now, the maximum number of comments is 5, including nested comments. I created a limit at 3 nested comments so no more levels should show. But I was still adding 2 top-level comments if the pagination setting allows it.

Yeah, I thought it was to illustrate that there might be more top-level comments. I think one such top-level comment would be enough to illustrate though, and at the end of the day, I don't even think that it's strictly needed at all.

Anyways, if it feels there are still too many I can remove the other top-level comments as you suggest to always show 3 comments at max.

Yeah, TBH, I'd rather save more space on screen 😅

This allows us to hard-wire the Comments Title placeholder back to "3"

Not sure about that. Imagine you have the pagination setting set to 2 comments per page and no nested comments, it would have 2 replies for example.

Not sure I'm following 🤔 Doesn't the Comments Title block show the total number of comments (across all pages)?

Or are you saying it would be confusing for the user if the Comments Title says "3 Comments" but we only show two because of pagination settings? 🤔 That's a fair point.

I'd go as far as to say that only 2 comments per page seems like a bit of an edge case (I'd assume most people would want to show more). However, it might make sense to adapt the Comments Title block logic to reflect the number of comments the user sees in that case 👍

To be honest, I don't have a strong opinion in this regard, so we can go your way.

Thanks for bearing with me! 😊

@SantosGuillamot
Copy link
Contributor Author

Okay, I've just pushed a commit to match the UX you defined 🙂 I believe both the comments and the title are working with all the settings. Let me know if there's something not working as expected or missing.

@ockham ockham self-assigned this Sep 28, 2022
@ockham
Copy link
Contributor

ockham commented Oct 5, 2022

tl;dr for triage: This PR is for editor parity with the changes made to the frontend of the Comments block in #44351, to reflect the discussion setting for comments threading (i.e. render comments unthreaded when the setting is disabled). With threading enabled, we'd always show three nested placeholder comments in the Site Editor; with threading disabled, we'd only show one (but still say "3 responses" in the Comments Title block placeholder).

With threading disabled:

image

In the editor:

trunk This branch
image image

With threading limited to two levels:

image

trunk This branch
image image

With threading limited to three or more levels:

image

trunk This branch
image image

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.

Thank you @SantosGuillamot! LGTM :shipit:

@ockham ockham merged commit 58be49f into trunk Oct 5, 2022
@ockham ockham deleted the fix/discussion-settings-in-comments-block-edit branch October 5, 2022 15:11
@github-actions github-actions bot added this to the Gutenberg 14.4 milestone Oct 5, 2022
ockham added a commit that referenced this pull request Oct 10, 2022
Respect discussion settings in the new Comments block while working in the Site Editor. We are loading some placeholders, and they should follow the discussion settings as much as possible in order to reflect the frontend output.

Co-authored-by: Bernie Reiter <[email protected]>
@ockham
Copy link
Contributor

ockham commented Oct 10, 2022

I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: b5c56fb

@ockham ockham removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 10, 2022
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Oct 11, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54483 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Oct 11, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54483


git-svn-id: http://core.svn.wordpress.org/trunk@54042 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Oct 11, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54483


git-svn-id: https://core.svn.wordpress.org/trunk@54042 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ootwch pushed a commit to ootwch/wordpress-develop that referenced this pull request Nov 4, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54483 602fd350-edb4-49c9-b593-d223f7449a82
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comments Affects the Comments Block - formerly known as Comments Query Loop Needs User Documentation Needs new user documentation [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants