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

Image: Adds the block controls for uploading image. #64320

Merged
merged 16 commits into from
Sep 11, 2024
Merged

Image: Adds the block controls for uploading image. #64320

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
22d2660
feat: Adds the block controls for uploading image.
vipul0425 Aug 7, 2024
dd4193b
feat: Adds the size based placeholder logic.
vipul0425 Aug 12, 2024
c0835cc
fix: The layout design for small images and adds grid support.
vipul0425 Aug 14, 2024
4b3edff
refactor: Remove resize observer and add lockURLControls support.
vipul0425 Aug 15, 2024
92f2e93
Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/issu…
vipul0425 Aug 20, 2024
ad4fbaa
refactor: Make use of the existing MediaReplaceFlow component.
vipul0425 Aug 28, 2024
f4c8b58
Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/issu…
vipul0425 Aug 28, 2024
61bc2bf
fix: The mediaPlaceholder location after merge.
vipul0425 Aug 28, 2024
911e938
fix: The initial flash in small size.
vipul0425 Aug 28, 2024
fe1c851
Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/issu…
vipul0425 Aug 31, 2024
19a8ccb
feat: Removes the redundant css and redefine the placeholder logic.
vipul0425 Aug 31, 2024
fca4521
Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/issu…
vipul0425 Sep 5, 2024
086ea22
fix: Adressed the PR feedbacks.
vipul0425 Sep 5, 2024
dd46ab8
fix: Removes the effective width logic and moves observer inside of p…
vipul0425 Sep 5, 2024
b7b14f5
fix: The e2e test failures
vipul0425 Sep 11, 2024
fdedc59
Update packages/block-library/src/image/edit.js
vipul0425 Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 70 additions & 30 deletions packages/block-library/src/image/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@ import { Placeholder } from '@wordpress/components';
import { useDispatch, useSelect } from '@wordpress/data';
import {
BlockIcon,
MediaPlaceholder,
useBlockProps,
MediaPlaceholder,
store as blockEditorStore,
__experimentalUseBorderProps as useBorderProps,
__experimentalGetShadowClassesAndStyles as getShadowClassesAndStyles,
useBlockEditingMode,
MediaReplaceFlow,
BlockControls,
__experimentalLinkControl as LinkControl,
} from '@wordpress/block-editor';
import { useEffect, useRef, useState } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';
Expand Down Expand Up @@ -107,7 +110,13 @@ export function ImageEdit( {
align,
metadata,
} = attributes;

const [ temporaryURL, setTemporaryURL ] = useState( attributes.blob );
// const parentWidth = sizes?.width;
// const parsedWidth = width && parseInt( width.replace( /px$/, '' ), 10 );

// const isSmallLayout =
// ( parentWidth && parentWidth < 160 ) || parsedWidth < 160;

const altRef = useRef();
useEffect( () => {
Expand Down Expand Up @@ -364,35 +373,66 @@ export function ImageEdit( {
};

return (
<figure { ...blockProps }>
<Image
temporaryURL={ temporaryURL }
attributes={ attributes }
setAttributes={ setAttributes }
isSingleSelected={ isSingleSelected }
insertBlocksAfter={ insertBlocksAfter }
onReplace={ onReplace }
onSelectImage={ onSelectImage }
onSelectURL={ onSelectURL }
onUploadError={ onUploadError }
context={ context }
clientId={ clientId }
blockEditingMode={ blockEditingMode }
parentLayoutType={ parentLayout?.type }
/>
<MediaPlaceholder
icon={ <BlockIcon icon={ icon } /> }
onSelect={ onSelectImage }
onSelectURL={ onSelectURL }
onError={ onUploadError }
placeholder={ placeholder }
accept="image/*"
allowedTypes={ ALLOWED_MEDIA_TYPES }
value={ { id, src } }
mediaPreview={ mediaPreview }
disableMediaButtons={ temporaryURL || url }
/>
</figure>
<>
<figure { ...blockProps }>
{ ! ( temporaryURL || url ) && ! lockUrlControls && (
<BlockControls group="other">
<MediaReplaceFlow
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
mediaId={ id }
mediaURL={ url }
allowedTypes={ ALLOWED_MEDIA_TYPES }
accept="image/*"
name={ __( 'Add image' ) }
onSelect={ onSelectImage }
onError={ onUploadError }
>
<form className="block-editor-media-flow__url-input has-siblings">
<span className="block-editor-media-replace-flow__image-url-label">
{ __( 'Insert from URL:' ) }
</span>

<LinkControl
value={ { url } }
settings={ [] }
showSuggestions={ false }
onChange={ ( value ) => {
onSelectURL( value.url );
} }
/>
</form>
</MediaReplaceFlow>
</BlockControls>
) }
<Image
temporaryURL={ temporaryURL }
attributes={ attributes }
setAttributes={ setAttributes }
isSingleSelected={ isSingleSelected }
insertBlocksAfter={ insertBlocksAfter }
onReplace={ onReplace }
onSelectImage={ onSelectImage }
onSelectURL={ onSelectURL }
onUploadError={ onUploadError }
context={ context }
clientId={ clientId }
blockEditingMode={ blockEditingMode }
parentLayoutType={ parentLayout?.type }
/>

<MediaPlaceholder
icon={ <BlockIcon icon={ icon } /> }
onSelect={ onSelectImage }
onSelectURL={ onSelectURL }
onError={ onUploadError }
placeholder={ placeholder }
accept="image/*"
allowedTypes={ ALLOWED_MEDIA_TYPES }
value={ { id, src } }
mediaPreview={ mediaPreview }
disableMediaButtons={ temporaryURL || url }
/>
</figure>
</>
);
}

Expand Down
26 changes: 21 additions & 5 deletions packages/block-library/src/image/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
// Provide special styling for the placeholder.
// @todo this particular minimal style of placeholder could be componentized further.
.wp-block-image.wp-block-image {

// Show Placeholder style on-select.
&.is-selected .block-editor-media-placeholder {
&.is-selected .block-editor-media-placeholder:not(.is-small) {
// Block UI appearance.
color: $gray-900;
background-color: $white;
Expand All @@ -27,13 +26,25 @@
&::before {
opacity: 0;
}

.block-bindings-media-placeholder-message {
opacity: 1;
}
}

.block-editor-media-placeholder.is-small {
min-height: 60px;
}

.block-editor-media-placeholder:not(.is-small) {
.components-placeholder__fieldset,
.components-placeholder__label {
display: flex;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

.components-* class names are meant to be a private implementation detail and should not be used outside of the components package.

Overrides like the one highlighter make this usage Placeholder component prone to future breakage, should the component update internally (either its classnames, or how its internal layout is organized).

Having said that, I can see that there are more examples of using private classnames and in general applying style overrides in this file, which is a strong indicator that we should completely rethink how the media placeholder is implemented. So, in that sense, my feedback shouldn't considered blocking for this specific PR, but rather a warning about how badly styles are architected for this component and the risks.

It feels like it's trying to bend the Placeholder component beyond its limits. Therefore:

  • we should determine if there are any features that we can add to the Placeholder component so that consumers don't have to resort to style overrides and other hacky practices;
  • otherwise, we should re-implement the component in a way that builds on top of Placeholder . For example, instead of setting display:none to .components-placeholder__illustration, why don't we pass the withIllustration={false} prop?
  • finally, if the designs / feature are diverging too much, we should ask ourselves whether this warrants a new, separate component (in the block library package)

}
.block-bindings-media-placeholder-message {
opacity: 0;
}
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
&.is-selected .block-bindings-media-placeholder-message {
opacity: 1;
}

// Remove the transition while we still have a legacy placeholder style.
// Otherwise the content jumps between the 1px placeholder border, and any inherited custom
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -43,6 +54,11 @@
.components-button {
transition: none;
}

.components-placeholder__fieldset,
.components-placeholder__label {
display: none;
}
}

figure.wp-block-image:not(.wp-block) {
Expand Down
Loading