-
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
Try dark toolbar for the write mode #66116
Try dark toolbar for the write mode #66116
Conversation
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. |
@richtabor can you please review this PR |
This is working decently well for me. Would love a design review to ensure it's inline what the original ideal. |
@@ -167,11 +171,13 @@ export function PrivateBlockToolbar( { | |||
// Shifts the toolbar to make room for the parent block selector. | |||
const classes = clsx( 'block-editor-block-contextual-toolbar', { | |||
'has-parent': showParentSelector, | |||
'is-dark-toolbar': ! isDefaultEditingMode && ! hasFixedToolbar, |
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'd avoid words that dictate the appearance (dark
) as we may need to re-evaluate the design when we introduce light/dark modes down the road.
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.
So how you would name this. See my previous comment here #66116 (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 could use inverted
or alt
(as in alternative
)?
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.
@dhruvang21 I think this line causes the issues in my video - isDefaultEditingMode
applies to indivudual blocks which can be locked or in partially synced patterns so we can end up with the dark toolbar in default mode.
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.
@draganescu, you’re absolutely right; this condition isn’t complete as it fails for blocks with unique states—like those that are locked, reusable, or partially synced in patterns—where each block’s mode can differ within the editor.
For now, I think we could add this condition: getBlockEditingMode(parentClientId) === 'contentOnly'
, which directly targets the parent block’s mode. However, a better approach might be to retrieve the editor mode in a way similar to how it’s done for zoom-out activation.
Let me know your thoughts!
I would echo Jay's note. There are some good ideas, good intents. But compent and admin-level theming is increasingly on the radar, which will afford a systematic approach to more easily test and tune this in a way that does not risk adding technical debt. I would circle back to this once we're a bit further. But thank you for the contribution, the instinct, and the work! |
My above comment aside, 66054 makes a good case for why a dark toolbar as outlined can make sense even before theming. If this passes a good code review, no blockers from my end. |
I certainly agree it's worth trying something to help indicate the active mode. I'm not 100% sold on using the toolbar to do that because it's not always visible, and because 'top toolbar' exists. That said I would also not block :) |
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.
Related, though not for this PR: #66455. |
We also need to fix the contrast on toggled buttons. |
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.
There is a bug in the implementation which keeps the light toolbar around after switching modes, plus the style leaks to all nstances of content only whereas we only want this for the write mode
dark-toolbar-leaks.mp4
Hello @draganescu, Could you please share the steps to recreate the issue you showed in the video? I wasn’t able to reproduce it when testing in the Gutenberg PR reviewer. |
Hi @dhruvang21 sorry for the delay, and thanks for your patience too! To repro the issues I encountered:
also
I think the problem is we don't target the mode but the "locking" and the issue is to use the dark toolbar in write mode only, eg. when the blocks are locked in content only keep the white toolbar, This PR also needs a rebase now as the write/design mode have been moved to an experiment. Hope this helps! |
10d338c
to
3fb856b
Compare
I fixed the issue raised by @oandregal above and addressed some feedback. I think this is ready to go. I don't love it personally but I'm ok shipping and trying it. |
Co-authored-by: dhruvang21 <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: hanneslsm <[email protected]>
What?
Solves #66054
Let's try adding an additional affordance to write mode to help you understand what "mode" you're in, writing or design, by leveraging the pre-existing styles for select mode. This way there's a clear visual affordance to indicate the difference in experiences.
Why?
Part of 66054
How?
I’ve applied the conditions for the
is-contentonly-mode
class, which gets added when the write mode is activated. I also tried to add the SCSS to match the design. While I did my best to ensure the SCSS is correct, I believe it can be further optimized through the review process if necessary.Testing Instructions
Screenshots or screencast
Dark-toolbar-mode.mp4