-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Block comments: Add comment indicators in the block toolbar #71271
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: Add comment indicators in the block toolbar #71271
Conversation
…nal commenters and repliers
…f background colors
|
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. |
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
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.
Thanks for the PR!
@WordPress/gutenberg-design, We'd appreciate your design feedback
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
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.
We should monitor REST API queries closely, as the collab sidebar is being introduced. It would also be nice to test on comment-heavy setups.
|
Updated ✅ |
|
I've looked into it a bit more, but would simply specifying |
|
Woah! I wasn't aware of this, might just do the trick 🚀 Perhaps the status values should be specified in documentation, currently it just says that the query's default value is |
…quests and update status to 'all' in CollabSidebar
|
|
Yes! Good point - 'all' will work for now. That said, I still feel we should add the multi-status support to the end point. The underlying comments class already supports arrays for the status field so the REST API lacking support is probably a bug/oversight. We may need more fine grained views of comments later where this could be useful. |
I agree, the block commenting might require additional comment status in the feature. That said, can we merge this PR for now? |
Sure. This isn't a blocker. |
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/collab-sidebar/comment-indicator-toolbar.js
Outdated
Show resolved
Hide resolved
adamsilverstein
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.
Looks good! left some tiny style feedback items.
…r-toolbar.js Co-authored-by: Adam Silverstein <adamsilverstein@earthboundhosting.com>
…r-toolbar.js Co-authored-by: Adam Silverstein <adamsilverstein@earthboundhosting.com>
…r-toolbar.js Co-authored-by: Adam Silverstein <adamsilverstein@earthboundhosting.com>
|
OK, it seems to be ready to ship. @WordPress/gutenberg-design, I believe this PR accurately implements the design proposed here, but if you have any design feedback, please leave a comment and we'll address it in a follow-up. |
What?
Closes #71243
This PR adds comment indicators on block toolbar
Indicator shows avatar of users participating in the thread
Also shows an orange dot indicating resolution status of the thread
I also intend to work on adding indicators to block list view after approach in this is finalized for consistency
Why?
Having enhanced indicators was one of the first suggestions on block comments features
An initial design for the same on Block Toolbar was provided in #66377 (comment)
How?
The comment icon button in toolbar is replaced with the new indicator, the indicator shows avatars of max 3 users participating in the thread, should there be more participants, another circle with "+1", etc text is added
The comments with
trashstatus are ignored so that deleted comments don't appear in the indicatorFollows mockup from #66377 (comment)
Testing Instructions
Screenshots or screencast
Before
After
With one user
With two users
With three users
With 3+ users
3 users with resolved status