Inline commenting: UX Enhancements for Comments #67385
Merged
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
akasunil
added
[Type] Enhancement
|
Size Change: +151 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…nt/inline-commenting-refactored-comments-component
|
Flaky tests detected in 9306f80. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12156256176
|
|
Hi @akasunil. Not sure if this is how it should work. When the thread is selected/focussed, the replies should be visible? I'm adding replies here, and they immediately disappear. 
Also, when a thread is focussed, I should see the reply form underneath. 
|
…nt/inline-commenting-refactored-comments-component
…nt/inline-commenting-refactored-comments-component
ellatrix
reviewed
Dec 4, 2024
ellatrix
reviewed
Dec 4, 2024
…nt/inline-commenting-refactored-comments-component
This was referenced Dec 4, 2024
|
@akasunil I tested this again. One odd behaviour: when I click on another block, it shouldn't leave the comment focussed. Could we also rename this PR, I think the title is a bit misleading, it's not just a refactor? |
ellatrix
reviewed
Dec 4, 2024
| @@ -18,13 +18,17 @@ | |||
| position: relative; | |||
| padding: $grid-unit-20; | |||
| border-radius: $radius-large; | |||
| border: 1px solid $gray-300; | |||
| border: 1.5px solid $gray-300; | |||
There was a problem hiding this comment.
not strong opinion here, We could change it to 1px or 2px, I updated it to make it same as active/focused state border to prevent layout shift on board active/focus state.
ellatrix
reviewed
Dec 4, 2024
| setActiveThread( blockCommentId ?? null ); | ||
| setFocusThread( null ); | ||
| setShowCommentBoard( false ); | ||
| }, [ blockCommentId ] ); |
There was a problem hiding this comment.
Generally, setting state in an effect like this is not a good idea. Maybe just unmount the component with a key so the local state within is automatically reset?
There was a problem hiding this comment.
The key suggestion does not work?
akasunil
changed the title
I've verified on my end, and it functions as seen in the attached video below. Some video recording might help me understand this strange behavior you are experiencing with the comment board. Edit-Post-.Hello-World-.-.-gutenberg-.-WordPress.1.webm |
|
@akasunil It seems to happen when I manually focus a comment of a block that is not selected. When I then click on another block, it doesn't disappear. I think when I focus a comment, it should probably select the relevant block, but that's addressed in another PR. |
ellatrix
approved these changes
Dec 4, 2024
There was a problem hiding this comment.
Would just like to see us avoid setting state in an effect: #67385 (comment)
…nt/inline-commenting-refactored-comments-component
I have addressed that.
I think we will have to revisit this issue after (#66873) PR is merged. |
|
Now focus does not seem to clear when I select a different blocks. It's now possible to have two comments focussed at the same time. This should never be possible? |
ellatrix
reviewed
Dec 5, 2024
| onAddReply( inputComment, thread.id ); | ||
| } } | ||
| onCancel={ ( event ) => { | ||
| event.stopPropagation(); // Prevent the parent onClick from being triggered |
There was a problem hiding this comment.
If we really need to do this, there must be really good reason for it. Why is there a click handler in a parent element. Is it focusable? Why not base comment focus on whether or not the wrapper or an inner focusable element is focussed?
There was a problem hiding this comment.
We are removing the board's focus when the cancel button is clicked which conflicts with the parent element click event. I added a focus event to the wrapper element and the focus state in the parent component because we require one board focused at a time.
This is partially fixed, There is still issue when board is focused on board click. But i guess that would be resolved by (#66873). |
akasunil
deleted the
enhancement/inline-commenting-refactored-comments-component
branch
yogeshbhutkar
pushed a commit
to yogeshbhutkar/gutenberg
that referenced
this pull request
Dec 18, 2024
* Refactored comments components and show replies on button click * Update board and show more reply text styles * Set chronological order for reply comments * Improved functionality for thread focus and active state and other fixes * Remove board highlight on block selection change * Close popover on new comment option click * Fix styles for new comment board * Fix reply order issue * Remove unnecessary state * Set initial value for focusThread state and remove use of useEffect * Fix component remount issue Co-authored-by: akasunil <[email protected]> Co-authored-by: ellatrix <[email protected]>
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.
Part of - #66377
What? & Why?
Refactored comments component for ease of use. and Implemented
show more repliesfunctionality on comment board. Reply comments will now be hidden by default and will be visible on board focus.Few more fixes/enhancements done in this PR are below:
Testing Instructions
Gutenberg->ExperimentsScreenshots or screencast
Edit-Post-.Hello-World-.-.-gutenberg-.-WordPress.webm