-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Inline Commenting: Added new sidebar as extension of the canvas #67347
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +233 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. |
Flaky tests detected in f1d7e1c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12049742545
|
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.
From the video, it looks like this canvas sidebar is not always present when there are comments. Could this be addressed? The sidebar should always be open if there are comments unless another sidebar is open.
const iframeBody = iframeDocument?.body; | ||
if ( iframeBody ) { | ||
const style = window.getComputedStyle( iframeBody ); | ||
return style?.backgroundColor; |
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 would be nice if we can just get the colour from global styles, but I guess this is fine for now.
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’ve updated the implementation to pull the color directly from the global styles., and also kept this function for fallback background color.
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.
Let's remove it for now, and reduce complexity. There's also cases where the editor doesn't have an iframe, so this code is too fragile atm.
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.
Got it, I'll remove the fallback function for now. However, we might still encounter cases where global styles aren't supported, should we consider that later?
…e-commenting-canvas-sidebar
…e-commenting-canvas-sidebar
Thanks for the feedback! Should be fine now, Edit-Post-.Hello-World-.-.-gutenberg-.-WordPress.2.webm |
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.
Great, thank you! Let's remove the fallback colour getting and merge.
enableComplementaryArea( 'core', 'edit-post/collab-sidebar' ); | ||
unsubscribe(); | ||
} | ||
} ); |
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 could probably create an option for PluginSidebar
to use the sidebar as a fallback sidebar that is always open instead of this, but it's fine as a temporary solution.
const backgroundColor = GlobalStyles?.styles?.color?.background; | ||
|
||
if ( 0 < resultComments.length ) { | ||
const unsubscribe = subscribe( () => { |
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.
Can you change this to an effect? We shouldn't subscribe on render.
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.
useEffect
won't work in this case, as the component doesn't re-render when another sidebar is toggled from outside the editor.
Part of - #66377
What? & Why?
To align with the new designs, one sidebar should be colored to provide a seamless extension of the canvas. This "sidebar" should appear anytime no other sidebars are active, such as the inspector, global styles, second comment sidebar, and so on.
How?
This PR adds a new sidebar that resembles an extension of canvas, with the same background color applied to it. This sidebar will open when you click the comment icon in the block toolbar.
Comment's order and locations relative to blocks will be addressed in separate PR.
Testing Instructions
Gutenberg
->Experiments
Screenshots or screencast
Edit-Post-.Hello-world-.-.-gutenberg-.-WordPress.1.webm