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

Omit "design" controls from Cover block toolbar while in Write mode #66853

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

Conversation

up1512001
Copy link
Member

What?

Based on editor mode updating cover block toolbar controls.

Why?

closes #66823

How?

  • get mode from the editor and based on that render or skip cover block toolbar controls.

Testing Instructions

  1. select write mode in editor
  2. add cover block and check its toolbar
  3. full screen & matrix controls won't be visible if write mode is selected.

Screenshots or screencast

Screen.Recording.2024-11-08.at.11.01.18.mov

@up1512001 up1512001 added [Type] Bug An existing feature does not function as intended [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Nov 8, 2024
@up1512001 up1512001 self-assigned this Nov 8, 2024
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: up1512001 <up1512001@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

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

@richtabor richtabor changed the title feature: based on editor mode render block toolbar controls for cover block Omit "design" controls from Cover block toolbar while in Write mode Nov 8, 2024
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

There are three editing modes: default, disabled, and content-only.

In my opinion, it's safer to check if the edit mode is default than to check if the edit mode is contentOnly.

If the edit mode is disabled, we shouldn't be able to select the block in the first place, but just to be safe, it's better to ensure that the controls aren't rendered even if the edit mode is disabled,

This means that we should be able to reuse the existing hasNonContentControls variable.

@up1512001
Copy link
Member Author

@t-hamano also added hasNonContentControls check, please review 🙇‍♂️

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

While this works, I think the main idea of "write mode" is that it shouldn't be the responsibility of the block to adapt itself.

So instead of this, I wonder if we can instead "hide the slots" from the block toolbar like we do for zoom-out mode.

@up1512001
Copy link
Member Author

@youknowriad for zoom-out we are hiding all slots like parent, block, inline, other, but I suppose we only need to hide block slot when "Write mode" is selected, we can do that by conditionally rendering block slot.
Also this will solve one issue that order of toobar controls are changing if write mode is selected & de-selected.

{ isDefaultEditingMode && (
	<BlockControls.Slot
	    group="block"
	    className="block-editor-block-toolbar__slot"
	/>
) }

But if we do this then there will be one problem that where ever in current implementation for "Write Mode" toolbar controls are added to block slot needs to be moded to inline or other.
like this image block write mode toolbar control needs to be changed otherwise it won't be visible.

<BlockControls group="block">

Let me know your thoughts 🙇‍♂️

@youknowriad
Copy link
Contributor

What's the reasoning for only hiding "block" toolbar controls but not the others? I think one thing that is clear is that we shouldn't be thinking about these issues (controls in write) in isolation. So how do we decide that a block toolbar makes sense or not in a block in a generic way?

cc @richtabor curious about your thoughts as you created these issues.

@up1512001
Copy link
Member Author

What's the reasoning for only hiding "block" toolbar controls but not the others?

If you see image block toolbar controls, some of them are added only for "Write mode," like Alternative text and Title text. So, if we hide all controls like "Zoom-out" mode, then these controls need to be moved to InspectorControl.

@youknowriad
Copy link
Contributor

If you see image block toolbar controls, some of them are added only for "Write mode," like Alternative text and Title text. So, if we hide all controls like "Zoom-out" mode, then these controls need to be moved to InspectorControl.

Yes, that makes sense, I'm just trying to understand what's the generic rule, we shouldn't have to implement specific checks like that for each block. We also need to think about third-party blocks and we shouldn't be forcing them to implement specific logic so I'm trying to see if there's a rule to be extracted or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove design-oriented controls from Cover block toolbar while in Write mode
4 participants