Repository navigation
Real-time Collaboration: Add collaborators presence UI - #75065
Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
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. |
ingeniumed
left a comment
There was a problem hiding this comment.
Nice work getting this in place. Most of my comments are about code cleanup and comments, but I do have one concern that needs to be tested to make sure it's not a problem.
| const isLargeViewport = useViewportMatch( 'medium' ); | ||
| const isTooNarrowForDocumentBar = useMediaQuery( '(max-width: 403px)' ); | ||
| const { | ||
| postId, |
There was a problem hiding this comment.
Looking at the postType, it looks like there are a lot of possible post types here. We are interested in mainly the postType for posts. We do handle if the awareness instance isn't found, and perhaps the active users would be empty for cases where the post type isn't post. What happens if this is triggered for cases where the post type isn't post? Is there any error or does it work as expected?
This looks like it might have more side effects, and I'm not sure what that'd look like.
There was a problem hiding this comment.
It's already using getCurrentPostType() here, I just added getCurrentPostId() which shouldn't be problematic. The hook will always just return the default/empty state if it doesn't find anything.
| <div | ||
| className={ className } | ||
| style={ avatarStyles } | ||
| aria-hidden="true" |
There was a problem hiding this comment.
Why is it marked with aria-hidden? Will we rely on collaborators-list for announceable text?
There was a problem hiding this comment.
This was done by @shekharnwagh originally, but I do believe the reason is because you don't want to go through this list and then also the dropdown list. You just want the items in the interactive list to be announced.
There was a problem hiding this comment.
Yeah, the text in collaborators-list is enough and this doesn't add meaningful content for screen readers
| * @param root0.postType | ||
| */ | ||
| export function CollaboratorsPresence( { postId, postType }: AvatarsProps ) { | ||
| const activeUsers = useActiveUsers( postId, postType ) as UserState[]; |
There was a problem hiding this comment.
harmful as. can you just rely on the return type, or is the type lost somehow?
| const activeUsers = useActiveUsers( postId, postType ) as UserState[]; | |
| const activeUsers = useActiveUsers( postId, postType ); |
There was a problem hiding this comment.
The type is lost because of the private API file. If there is a way to preserve it, I'd definitely like to
| className="editor-collaborators-presence__list-item" | ||
| onClick={ () => {} } | ||
| disabled={ ! userState.isConnected } | ||
| aria-label="Clicking scrolls to cursor position in the editor" |
There was a problem hiding this comment.
Does it? This button is a no-op, can it just be removed?
There was a problem hiding this comment.
Removed the label and disabled the button per Gopal's review. Wanted to leave the button because the cursor registry stuff is coming back in a follow-up PR
| } | ||
|
|
||
| /** | ||
| * Hook to get the active users for a post editor. |
There was a problem hiding this comment.
Change users -> collaborators. This applies to the docs, as well as the return type. The return type should be ActiveCollaborator.
| postId, | ||
| postType, | ||
| }: CollaboratorsPresenceProps ) { | ||
| const activeUsers = useActiveCollaborators( |
|
Noting that because auto-merge was enabled, this got merged in. But a follow up PR is needed to change |
|
I was testing the feature and noticed a couple of bugs, or at least unexpected results.
P.S. Let me know if I should create a separate issue. ScreencastCleanShot.2026-02-05.at.17.06.14.mp4 |
Hey @Mamaduka , thanks so much for that report. We happen to have just replicated that bug and are tracking down a fix. An issue would be welcome but not necessary. |
|
Thanks, @chriszarate! Feel free to ping me for testing or review. |
Co-authored-by: maxschmeling <maxschmeling@git.wordpress.org> Co-authored-by: ingeniumed <ingeniumed@git.wordpress.org> Co-authored-by: chriszarate <czarate@git.wordpress.org> Co-authored-by: shekharnwagh <shekharnwagh@git.wordpress.org>
What?
Adds the collaborators presence UI in the header. Closes #74708
Why?
When there are other connected collaborators in a real-time collaboration session, the current user needs to be able to know who else is in the post.
How?
Uses the
useActiveUsershook added in #75009 to populate data in a new<CollaboratorsPresence>component.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast