-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Block Comments: Highlight the related block #71308
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 Comments: Highlight the related block #71308
Conversation
This reverts commit 63f81c1.
|
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. |
| selectBlock( relatedBlocks[ 0 ] ); | ||
|
|
||
| const iframe = document.querySelector( | ||
| 'iframe[name="editor-canvas"]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selecting and manipulating elements in the DOM this way feels fragile. Did you explore using a block attribute / datastore to tie the comment to the block/highlighting?
Perhaps we could leverage the existing mechanism that ties between blocks and comments - clicking on a block highlights the related comment thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsilverstein - Updated the feedback.
REC-20250826195838.mp4
Since |
Got it @t-hamano. I will create a new issue and a PR for the |
Mamaduka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple more notes. I think we're close to finishing this.
| const [ focusThread, setFocusThread ] = useState( | ||
| showCommentBoard && blockCommentId ? blockCommentId : null | ||
| ); | ||
|
|
||
| // Sync comment ↔ block highlighting bidirectionally. | ||
| useEffect( () => { | ||
| // Highlight comment when block is selected. | ||
| if ( blockCommentId && ! focusThread ) { | ||
| setFocusThread( blockCommentId ); | ||
| } | ||
| }, [ blockCommentId, focusThread, blocks, setFocusThread ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar sync effects can be tricky. I know there are "checks" in place, but this effect will run a lot of times. Each block update will trigger the effect.
IMO, we have a clear use, even "click on a comment element", there should be no need for any side-effects. Technically, we should be able to house all required logic in handleCommentSelect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mamaduka – Removing the useEffect will stop highlighting comments when any block is selected, but the block flashing when a comment is selected will continue to work as expected. Do you think it’s fine if I proceed with this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a big blocker; we can start here and refactor later.
I just noticed that the blocks dependency isn't required for this effect; what was the reason for adding it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its just for syncing the highlight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like an odd hack.
| // Handle comment selection. | ||
| const handleCommentSelect = useCallback( | ||
| ( threadId ) => { | ||
| // Clear the add comment form when selecting a comment. | ||
| setShowCommentBoard( false ); | ||
|
|
||
| // Set the focused thread. | ||
| setFocusThread( threadId ); | ||
|
|
||
| // Flash the related block when a comment is clicked. | ||
| if ( blocks ) { | ||
| const relatedBlockClientId = findBlockByCommentId( | ||
| threadId, | ||
| blocks | ||
| ); | ||
| if ( relatedBlockClientId ) { | ||
| flashBlock( relatedBlockClientId ); | ||
| } | ||
| } | ||
| }, | ||
| [ blocks, flashBlock, setFocusThread, setShowCommentBoard ] | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need two handleCommentSelect functions? The logic should be co-located in a single callback below.
P.S. Memoization is sort of useless here. The blocks dependency will often change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mamaduka - Updated both the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should've been clearer. What I was suggesting was moving all the logic into handleCommentSelect defined in the Thread component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mamaduka - Thanks for the clarification! I've moved all the logic into the handleCommentSelect function defined in the Thread component as you suggested.
Changes made:
✅ Removed the handleCommentSelect function from the main Comments component
✅ Consolidated all logic (clearing comment board, setting focus, flashing blocks, scrolling) into the single handleCommentSelect in the Thread component
✅ Removed the useEffect for comment ↔ block highlighting sync
✅ Removed unnecessary useCallback memoization
The code is now much cleaner with a single source of truth for comment selection logic, and no performance issues from the frequently changing blocks dependency in memoization.
| // Flash the related block when a comment is clicked. | ||
| if ( blocks ) { | ||
| const relatedBlockClientId = findBlockByCommentId( | ||
| threadId, | ||
| blocks | ||
| ); | ||
| if ( relatedBlockClientId ) { | ||
| // Use longer timeout for comment-to-block highlighting (800ms) | ||
| flashBlock( relatedBlockClientId, 800 ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just use the relatedBlock value here. There's no need to find the same value twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mamaduka - Yes done.
| // Sync comment ↔ block highlighting bidirectionally. | ||
| useEffect( () => { | ||
| // Highlight comment when block is selected. | ||
| if ( blockCommentId && ! focusThread ) { | ||
| setFocusThread( blockCommentId ); | ||
| } | ||
| }, [ blockCommentId, focusThread, blocks, setFocusThread ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were keeping this effect for now? Was the removal intentional?
Sorry, just saw this mentioned in #71308 (comment). But it's still not clear if the removal was intentional.
Related: #71308 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mamaduka - Not intentional. Added back.
| // Flash the related block when a comment is clicked. | ||
| if ( relatedBlock ) { | ||
| flashBlock( relatedBlock ); | ||
| } | ||
|
|
||
| // Scroll to the related block element if available. | ||
| if ( relatedBlock && relatedBlockElement ) { | ||
| relatedBlockElement.scrollIntoView( { | ||
| behavior: 'instant', | ||
| block: 'center', | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should combine these so that they can "flash" the block highlight after the scroll.
P.S. Most of the inline comments inside the handleCommentSelect function aren't needed, IMO. What each part of the code is doing is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mamaduka - Good point! I've combined the flash and scroll so the block highlights after scrolling, and remove the unnecessary inline comments since the code is self-explanatory. Thank You.
t-hamano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you so much for sticking with it until the end. I believe this is the best we can do for now. If necessary, let's follow up by removing unnecessary effects, improving performance, and refactoring.
|
Nice work here @karthick-murugan - thanks for sticking with this thru all the feedback (and thanks @t-hamano and @Mamaduka for all the helpful guidance), this was a tricky issue to get working well. |
* Image size fix in lightbox * Revert "Image size fix in lightbox" This reverts commit 63f81c1. * Highlight and scroll to related blocks * Feedback changes updated * Latest Feedback updated * Feedback changes updated * Feedback changes added * Feedback changes * Feedback changes * Remove unecessarunnecessary effect and new api * Fixed feedback issues * Added flashblock and updated highlight issues * Update packages/editor/src/components/collab-sidebar/comments.js Co-authored-by: Adam Silverstein <[email protected]> * Feedback changes * Feedback changes updated * Update packages/editor/src/components/collab-sidebar/comments.js Co-authored-by: Adam Silverstein <[email protected]> * Flashblock and scroll updates added * Removing FlashBlock API changes * Remove duplicate handleCommentSelect * handleCommentSelect to Thread Component * Updating feedback * FlashBlock after scroll --------- Co-authored-by: karthick-murugan <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: adamsilverstein <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: joedolson <[email protected]> Co-authored-by: Sahil1617 <[email protected]> Co-authored-by: joemcgill <[email protected]> Co-authored-by: jeffpaul <[email protected]>
What?
Closes #71244
This PR introduces a visual highlight for the related block when a comment is selected in the sidebar. It ensures that users can quickly identify which block a comment is associated with inside the editor.
Why?
Currently, when comments are shown in the sidebar, there is no clear indication in the canvas of which block they belong to. This creates confusion and slows down workflows for authors and reviewers.
By highlighting the related block whenever its associated comment is selected, users will have a clear visual indicator of context, improving the overall commenting experience.
How?
When a comment is selected:
The related block is programmatically selected and a temporary data-comment-highlight="true" attribute is applied to the block’s DOM element inside the editor iframe.
A CSS rule applies a subtle highlight style to the block (e.g., outline or background fade).
When the comment is deselected or another block/comment is selected:
The data-comment-highlight attribute is removed, restoring the block to its normal appearance.
Implementation details
Uses iframe.contentDocument.querySelector('[data-block="..."]') to locate the block.
Adds data-comment-highlight="true" instead of class manipulation, since class selectors weren’t applying reliably inside the editor iframe.
Adds scoped CSS targeting [data-comment-highlight="true"] to visually differentiate highlighted blocks.
Testing Instructions
Open a post or page in the block editor.
Add a few blocks and insert comments linked to those blocks.
From the sidebar, click on a comment.
✅ The related block in the editor should receive a visible highlight.
Screenshot/ Video
REC-20250822175358.mp4