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.

Sign up for GitHub

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

Jump to bottom

Omit "design" controls from Image block toolbar while in write mode #66880

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

Omit "design" controls from Image block toolbar while in write mode #66880

wants to merge 8 commits into from

Conversation

up1512001
Copy link
Member

What?

Based on editor mode updating image block toolbar controls.

Why?

closes #66875

How?

  • conditional rendering block toolbar based on editor mode.

Testing Instructions

  1. open editor and insert image block.
  2. change editor mode and observer image toolbar controls will change based on editor mode.

Screenshots or screencast

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

@up1512001 up1512001 added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block labels Nov 8, 2024
@up1512001 up1512001 requested a review from richtabor November 8, 2024 17:30
@up1512001 up1512001 self-assigned this Nov 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 8, 2024 •
edited
Loading

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 <[email protected]>
Co-authored-by: talldan <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: richtabor <[email protected]>

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

@up1512001 up1512001 mentioned this pull request Nov 8, 2024
Open
@up1512001 up1512001 requested a review from t-hamano November 8, 2024 18:56
talldan
talldan reviewed Nov 11, 2024
View reviewed changes
packages/block-library/src/image/image.js Outdated
Comment on lines 735 to 750
<ImageURLInputUI
url={ href || '' }
onChangeUrl={ onSetHref }
linkDestination={ linkDestination }
mediaUrl={
( image && image.source_url ) || url
}
mediaLink={ image && image.link }
linkTarget={ linkTarget }
linkClass={ linkClass }
rel={ rel }
showLightboxSetting={ showLightboxSetting }
lightboxEnabled={ lightboxChecked }
onSetLightbox={ onSetLightbox }
resetLightbox={ resetLightbox }
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the option to link an image should still be present in Write (or content only) mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

@talldan address your feedback.

@youknowriad
Copy link
Contributor

Similar to the other PR. Is there a way to solve this without having the knowledge of the block editing mode within the block itself? (See question here #65699 (comment) and reply)

@ramonjd
Copy link
Member

ramonjd commented Nov 15, 2024 •
edited
Loading

Is there a way to solve this without having the knowledge of the block editing mode within the block itself?

So rather passed to the block somehow?

Block editing mode has been used in the image block since its creation, and in the meantime is used in more places than just navigation and image:

grep -rl 'useBlockEditingMode()' ./packages/block-library/src
./packages/block-library/src/paragraph/edit.js
./packages/block-library/src/heading/edit.js
./packages/block-library/src/navigation/edit/index.js
./packages/block-library/src/template-part/edit/inner-blocks.js
./packages/block-library/src/image/edit.js
./packages/block-library/src/button/edit.js
./packages/block-library/src/post-featured-image/edit.js
./packages/block-library/src/media-text/edit.js
./packages/block-library/src/post-title/edit.js
./packages/block-library/src/cover/edit/index.js

And there are three states: 'disabled', 'contentOnly', and 'default' so blocks might need to behave differently according to the value.

What if it were passed down through useBlockProps?

diff --git a/packages/block-editor/src/components/block-list/use-block-props/index.js b/packages/block-editor/src/components/block-list/use-block-props/index.js
index 25b9a21f0d..b30d9889e5 100644
--- a/packages/block-editor/src/components/block-list/use-block-props/index.js
+++ b/packages/block-editor/src/components/block-list/use-block-props/index.js
@@ -161,6 +161,7 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {
 		'data-block': clientId,
 		'data-type': name,
 		'data-title': blockTitle,
+		'data-editing-mode': blockEditingMode,
 		inert: isSubtreeDisabled ? 'true' : undefined,
 		className: clsx(
 			'block-editor-block-list__block',

It's a good discussion to have! Is it a blocker for this PR though?

If a better way to communicate block editing mode to blocks comes along, migrating affected blocks in the block library will have to be done anyway.

@youknowriad
Copy link
Contributor

You're right, it doesn't seem like a blocker for the current PR.

t-hamano
t-hamano reviewed Nov 15, 2024
View reviewed changes
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
@up1512001
update: as per suggestion
90f18ab 
Co-authored-by: t-hamano <[email protected]>
@up1512001
Copy link
Member Author

@t-hamano addressed your feedback, please check now.

@up1512001 up1512001 requested a review from t-hamano November 15, 2024 16:44
@up1512001 up1512001 added [Type] Enhancement A suggestion for improvement. and removed [Type] Bug An existing feature does not function as intended labels Nov 15, 2024
t-hamano
t-hamano reviewed Nov 16, 2024
View reviewed changes
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
packages/block-library/src/image/image.js Outdated Show resolved Hide resolved
@up1512001 up1512001 requested a review from t-hamano December 27, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramonjd ramonjd ramonjd left review comments

@talldan talldan talldan left review comments

@richtabor richtabor Awaiting requested review from richtabor

@artemiomorales artemiomorales Awaiting requested review from artemiomorales artemiomorales is a code owner

@t-hamano t-hamano Awaiting requested review from t-hamano

At least 1 approving review is required to merge this pull request.

Assignees

@up1512001 up1512001

Labels
[Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Image block should not have the "Add text over image" control when in Write mode
5 participants
@up1512001 @youknowriad @ramonjd @talldan @t-hamano