[Comments Query Loop] Show placeholder comments on site editor #38072
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +188 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
cbravobernal
requested review from
luisherranz,
gziolo,
michalczaplinski and
DAreRodz
|
As simple as that? 😄 I'll have a closer look tomorrow. |
gziolo
added
[Block] Comments
It seems so, but I don't know if is a good approach or we usually do setting empty objects as default in the project 😅 |
|
Hm.., interesting! It would require a bit more changes though, for example here as it throws a React warning right now. |
Using null as id would remove the warning. Anyway, the "options" are not working, and of course, if you set more than 1 item per page. It crashes (not very badly, though). I will iterate it more. |
cbravobernal
force-pushed
the
update/show-placeholder-comments-no-context-id
branch
from
afe35ac to
95b42ad
Compare
cbravobernal
commented
Jan 25, 2022
| @@ -97,7 +102,7 @@ const CommentsList = ( { | |||
| { comments && | |||
| comments.map( ( comment ) => ( | |||
| <BlockContextProvider | |||
| key={ comment.commentId } | |||
| key={ comment.commentId || uniqueId( 'comment-default' ) } | |||
There was a problem hiding this comment.
This way, if we have a null id (required for loading placeholders and not calling REST API for comments), we don't have an error or warning regarding the unique key inside a list. Not sure if it is the best approach.
There was a problem hiding this comment.
It seems that this approach is preventing the block to be selected.
Edited: it was a lack of a parent div with the blockprops, now it works fine.
There was a problem hiding this comment.
I think we need to apply a fallback key as part of comment object to ensure those keys don't change for the individual items. Otherwise, we will have unnecessary rerenders happening too often.
There was a problem hiding this comment.
Can we use the comments.map()'s index as a fallback?
Suggested change
| key={ comment.commentId || uniqueId( 'comment-default' ) } | |
| key={ comment.commentId || index } |
It's only considered an antipattern if individual items can be removed or moved around, which shouldn't be the case for placeholders IIUC.
cbravobernal
commented
Jan 25, 2022
| // to inner blocks to render the default placeholders. | ||
| if ( ! postId ) { | ||
| // We set a limit in order not to overload the editor of empty comments. | ||
| const defaultCommentsToShow = perPage <= 3 ? perPage : 1; |
There was a problem hiding this comment.
Is there any recommended number of comments placeholders to show at maximum?
Ping design @kjellr @jameskoster @karmatosed 😅
There was a problem hiding this comment.
Good question. My initial thought is to check thread_comments_depth and display enough placeholders to demonstrate maximum nesting.
There was a problem hiding this comment.
thread_comments_depth can go up to 10 though so that might be overkill 😅
I've settled on a somewhat arbitrary maximum nesting of 3 for the placeholders. I think that should make the threading clear without overwhelming the UI.
There was a problem hiding this comment.
I don't know, I think that can be important. For example if you want to support a depth of 10, you may need to adjust the margins in order for that level of nesting to work. If you don't see a preview then you wouldn't be aware of such an issue.
cbravobernal
force-pushed
the
update/show-placeholder-comments-no-context-id
branch
from
bb5ba90 to
4e2dbce
Compare
13 tasks
cbravobernal
changed the title
Open
2 tasks
cbravobernal
force-pushed
the
update/show-placeholder-comments-no-context-id
branch
from
4e2dbce to
e5127d7
Compare
8 tasks
Comment on lines
140
to
149
| rawComments: Array( defaultCommentsToShow ).fill( { | ||
| id: null, | ||
| } ), |
There was a problem hiding this comment.
I was quite sure that this would fail if defaultCommentsToShow were larger than 1, but I was wrong.
My concern was that convertToTree would fail in that case, but it handles it just fine 👍
There was a problem hiding this comment.
Let's see how will work with nested default comments (as it searches for a parent id) 😅
cbravobernal
force-pushed
the
update/show-placeholder-comments-no-context-id
branch
from
0967fd2 to
f4fd761
Compare
I added again the background color so no deprecations are needed 😄 I think the PR is ready to land. I will now work on an Avatar block (working wherever we put them). |
|
I'm seeing settings for blocks inside Comment Template repeated three times 😅 |
That is already happening before the PR 😓 I added an issue, but it also was mentioned in this comment I will work on that as soon as possible. |
I left the background option for not having to deprecate anything 😄 |
cbravobernal
force-pushed
the
update/show-placeholder-comments-no-context-id
branch
2 times, most recently
from
1ca9bd1 to
d193676
Compare
gziolo
reviewed
Feb 9, 2022
| 'pageComments' => get_option( 'page_comments' ), | ||
| 'threadComments' => get_option( 'thread_comments' ), | ||
| 'threadCommentsDepth' => get_option( 'thread_comments_depth' ), | ||
| 'avatarURL' => get_avatar_url( |
There was a problem hiding this comment.
I checked the Discussion Settings page and it looks like it has a ton of setting including the default avatar. That was surprising 😄
There was a problem hiding this comment.
Yep, most surprising is that the default avatar of discussion settings is the same default avatar selected for post authors.
gziolo
approved these changes
Feb 9, 2022
| ]; | ||
| } | ||
| } | ||
|
|
||
| if ( ! rawComments ) { |
There was a problem hiding this comment.
I think we could move this check just after the useSelect call that returns rawComments to avoid unnecessary computations. I also see useMemo hook call, so it would need to be after that.
Comment on lines
+215
to
+222
| // In case that `threadCommentsDepth` is falsy, we default to a somewhat | ||
| // arbitrary value of 3. | ||
| // In case that the value is set but larger than 3 we truncate it to 3. | ||
| const commentsDepth = Math.min( threadCommentsDepth || 3, 3 ); | ||
|
|
||
| // We set a limit in order not to overload the editor of empty comments. | ||
| const defaultCommentsToShow = | ||
| perPage <= commentsDepth ? perPage : commentsDepth; |
There was a problem hiding this comment.
This can be moved to the scope of if section as it's used only when postId is falsy. I commented about that earlier, all the logic for generating the tree of comment placeholders could be in their own function so it documents better.
This implementation improves the previous one in that it does not call the REST API with non-existent comment IDs
cbravobernal
force-pushed
the
update/show-placeholder-comments-no-context-id
branch
from
d193676 to
d012330
Compare
cbravobernal
deleted the
update/show-placeholder-comments-no-context-id
branch
cbravobernal
changed the title
22 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a quick approach to #37143. We are using the default placeholders' fallback of all components of a comment template.
A better solution may be to update all fallbacks to not depend on getting an empty comment object, feel free to comment or discuss if we should work on a better approach.
How has this been tested?
Screenshots
Screenshots were done on site editor.
Before:
After:
Types of changes
New possible feature discussed on #37143
Checklist: