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

Selecting on a comment should scroll to the block #66873

Open
wants to merge 22 commits into
base: trunk
Choose a base branch
from

Conversation

karthick-murugan
Copy link
Contributor

What?

Part of #66377

Why?

Focussing/selecting on a comment should scroll to the block that is being commented on.

How?

Added scroll to the editor section when the corresponding comments is selected.

Testing Instructions

  1. Create a page or post with some content.
  2. Add contents to the page such that the page is scrollable.
  3. Add inline comments to some of the blocks.
  4. Now Select the comments from the right side.
  5. The page will scroll to the corresponding blocks.

Screenshots or screencast

REC-20241108195829.mp4

Copy link

github-actions bot commented Nov 8, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: karthick-murugan <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: ellatrix <[email protected]>
Co-authored-by: akasunil <[email protected]>
Co-authored-by: Mamaduka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 8, 2024
Copy link

github-actions bot commented Nov 8, 2024

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @karthick-murugan! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@akasunil akasunil added [Type] Enhancement A suggestion for improvement. Collaborative Workflows Phase 3 of the Gutenberg roadmap around all-things related to collaborative workflows labels Nov 12, 2024
if ( block ) {
const blockClientId = block.clientId;
const iframe = document.querySelector(
'iframe[name="editor-canvas"]'
Copy link
Member

Choose a reason for hiding this comment

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

Let's use useBlockElementRef

Copy link
Contributor Author

@karthick-murugan karthick-murugan Nov 19, 2024

Choose a reason for hiding this comment

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

@ellatrix - when I try to use the useBlockElementRef hook, As per my understanding, it’s a part of the @wordpress/block-editor package. However, when I attempt to use it in the @wordpress/editor package, I cannot get it to work as expected.

Could you please confirm if this is an expected limitation? If so, are there any recommended workarounds or alternative approaches to achieve similar functionality (e.g., getting a reference to a block’s DOM element within the editor context)?

Thanks in advance.

Copy link
Member

Choose a reason for hiding this comment

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

You mean that it's not available as an API? Can we export it as a private API so we can use it?

Copy link
Member

@akasunil akasunil Nov 26, 2024

Choose a reason for hiding this comment

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

If just need to focus the respective block when comment board is focused, then I think below code would do the work

dispatch( 'core/block-editor' ).selectBlock( block?.clientId );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akasunil - thank you and I have updated it and pushed the code.

const clientID = select( blockEditorStore ).getSelectedBlockClientId();
return (
const clientBlocks = select( blockEditorStore ).getBlocks();
setBlocksList( clientBlocks );
Copy link
Member

Choose a reason for hiding this comment

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

Please don't set sate inside useSelect, return the value instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix - now the value has been returned inside useSelect

if ( block ) {
const blockClientId = block.clientId;
// Select the block in the editor
dispatch( blockEditorStore ).selectBlock( blockClientId );
Copy link
Member

Choose a reason for hiding this comment

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

Please use useDispatch instead, so we get the actions from the correct registry based on the context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix - Updated with useDispatch as per your advice.

);

useEffect( () => {
setBlocksList( selectedClientBlocks );
Copy link
Member

Choose a reason for hiding this comment

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

Why the extra local state here? It's not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line setBlocksList( selectedClientBlocks ); ensures the local blocksList state stays synchronized with the selected blocks from useSelect, enabling efficient block-related operations. Without it, the component would rely directly on selectedClientBlocks, risking inconsistent data or unnecessary re-renders.

Copy link
Member

Choose a reason for hiding this comment

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

Without it, the component would rely directly on selectedClientBlocks, risking inconsistent data or unnecessary re-renders.

Can you elaborate more about this or how to reproduce the issue?

As far as I can see, there's no need to keep a copy of selectedClientBlocks in a state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix and @Mamaduka , Yes I have removed the extra local state and updated the code.

@@ -64,14 +66,41 @@ export function Comments( {
setIsConfirmDialogOpen( false );
};

const blockCommentId = useSelect( ( select ) => {
const blockCommentId = useSelect( () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think you removed the select on accident here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error 'select' is already declared in the upper scope on line 20 column 34 no-shadow

@ellatrix - I removed the select to fix the above linting issue.

Copy link
Member

Choose a reason for hiding this comment

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

You should remove the select import above, and then you can pass select as an argument.

Example: useSelect( ( select ) => {}, [] );

@@ -46,6 +46,8 @@ export function Comments( {
} ) {
const [ actionState, setActionState ] = useState( false );
const [ isConfirmDialogOpen, setIsConfirmDialogOpen ] = useState( false );
// eslint-disable-next-line no-unused-vars
const [ activeClientId, setActiveClientId ] = useState( null );
Copy link
Member

Choose a reason for hiding this comment

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

Why is eslint disabled here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix - The above lines no longer needed and removed it.

@@ -35,7 +35,7 @@ import CommentForm from './comment-form';
* @param {Function} props.onAddReply - The function to add a reply to a comment.
* @param {Function} props.onCommentDelete - The function to delete a comment.
* @param {Function} props.onCommentResolve - The function to mark a comment as resolved.
* @return {React.ReactNode} The rendered Comments component.
* @return {JSX.Element} The rendered Comments component.
Copy link
Member

Choose a reason for hiding this comment

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

Why did this need to change? Just curious? What are we using elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix - Actually it was updated automatically when running the prettier command. Now replaced it back.

@@ -17,7 +17,7 @@ import {
} from '@wordpress/components';
import { Icon, check, published, moreVertical } from '@wordpress/icons';
import { __, _x } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import { useSelect, useDispatch, select } from '@wordpress/data';
Copy link
Member

Choose a reason for hiding this comment

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

Core code should avoid using global select and dispatch. Use static selector getter instead.

const { getBlocks } = useSelect( blockEditorStore );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mamaduka - removed the global select and updated as such.

Copy link
Member

Choose a reason for hiding this comment

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

@karthick-murugan, sorry for the confusion. I only suggested getting the getBlocks selector. You can leave the existing useSelect for the block comment ID as it was.

Usually, you'll use this method only to get selectors used in event callbacks like handleThreadClick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Mamaduka for the clarification. Updated as such.

Comment on lines +81 to +83
const [ blocks ] = useEntityBlockEditor( 'postType', postType, {
id: postId,
} );
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for switching to useEntityBlockEditor? This reverts the suggestion from #66873 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mamaduka - used useEntityBlockEditor for the functionality to work in Site Editor. Before it was not.

}
}
}
return blocks ? getBlockByCommentId( blocks, commentId ) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mamaduka - I have updated the condition now.

@Mamaduka
Copy link
Member

Mamaduka commented Dec 5, 2024

Maybe we should block the clientId part of the comment object? That would reduce the logic needed for this PR/feature.

We're already manipulating and building a comment tree in CollabSidebar - #66927.

@akasunil, @ellatrix, what do you think?

@akasunil
Copy link
Member

akasunil commented Dec 5, 2024

Maybe we should block the clientId part of the comment object? That would reduce the logic needed for this PR/feature.

I agree; I had this in mind because client ID linked to the comment ID is required through out commenting functionality. This would definitely reduce some logic.

@Mamaduka
Copy link
Member

Mamaduka commented Dec 5, 2024

@akasunil, this also would be useful for #67622.

@akasunil
Copy link
Member

akasunil commented Dec 5, 2024

@akasunil, this also would be useful for #67622.

Created PR #67630 to address this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Collaborative Workflows Phase 3 of the Gutenberg roadmap around all-things related to collaborative workflows First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants