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

Block Library - Gallery: Set 'addToGallery' prop to false when images don't have IDs #30122

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Block Library - Gallery: Set 'addToGallery' prop to false when images don't have IDs #30122

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions packages/block-library/src/gallery/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,11 @@ function GalleryEdit( props ) {
}, [ linkTo ] );

const hasImages = !! images.length;
Copy link
Contributor

@gwwar gwwar Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for taking this one! We can also filter out any unexpected data in the array. This can help guard against malformed data (potentially from bugs/accidental user-edits).

const imageIds = images.filter( ( id ) => ( Number.isInteger( id ) && id > 0 ) ); // for example [null,1,null,-3,44] will return [1, 44]
const hasImages = !! images.length;

//...
addToGallery={ hasImages }
value={ imageIds }

@glendaviesnz since you've been 👀 the Gallery block refactor, is it expected that the pattern in two images side-by-side has serialized null ids? If not, do you have a recommendation for a better spot to validate values here?

Twenty Twenty-One, Gallery Two Images Side by Side
Screen Shot 2021-04-02 at 8 41 25 AM 
<!-- wp:gallery {"ids":[null,null],"linkTo":"none","align":"wide"} -->
<figure class="wp-block-gallery alignwide columns-2 is-cropped"><ul class="blocks-gallery-grid"><li class="blocks-gallery-item"><figure><img src="https://s.w.org/images/core/5.5/don-quixote-05.jpg" alt="An old pencil drawing of Don Quixote and Sancho Panza sitting on their horses, by Wilhelm Marstrand."/></figure></li><li class="blocks-gallery-item"><figure><img src="https://s.w.org/images/core/5.5/don-quixote-01.jpg" alt="An old pencil drawing of Don Quixote and Sancho Panza sitting on their horses, by Wilhelm Marstrand."/></figure></li></ul></figure>
<!-- /wp:gallery -->

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @gwwar, for the review.

We can also filter out any unexpected data in the array.

It makes sense.

I think we should keep image IDs check separated. While patterns might not have IDs, technically, images are there, and we want to have an edit component in that state.

Copy link
Contributor

@glendaviesnz glendaviesnz Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

is it expected that the pattern in two images side-by-side has serialized null ids?

Yes, this is the way the gallery handles external images, eg. if you add a stand alone image block and choose the option to add it from url, and then transform the image block to a gallery block it will add a null entry to the ids array.

do you have a recommendation for a better spot to validate values here?

I had a quick look at it appears like the ideal fix for this will need to go in the core media browser, as it needs to know that if it is passed an addToGallery=true, but an empty value, to assume that a gallery with external images is being updated, and in which case it would need to display those images in the gallery edit screen, even though they are not saved in the media library.

Currently @Mamaduka your fix prevents all of the images being added by default, but the media library defaults to 'Create gallery' which means that the existing images are overwritten:

gallery

This is ok with the patterns use case, as we can probably assume the user wants to replace the placeholder images, but as mentioned above it is possible for a user to create their own gallery with null image ids, in which case their images will be lost if they try to add additional images from the media library.

I had a quick look and couldn't see a way around this from the block side of things. I will have a look at the core side and see what options there might be, but not familiar with that code so not sure how far I will get.

Copy link
Contributor

@glendaviesnz glendaviesnz Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

in which case their images will be lost if they try to add additional images from the media library.

this is the current behaviour anyway I just realised, ie. if a user edits a gallery with null id images their existing images are lost, so we wouldn't make things any worse by at least not adding all images by default.

const hasImageIds = hasImages && images.some( ( image ) => !! image.id );

const mediaPlaceholder = (
<MediaPlaceholder
addToGallery={ hasImages }
addToGallery={ hasImageIds }
isAppender={ hasImages }
disableMediaButtons={ hasImages && ! isSelected }
icon={ ! hasImages && sharedIcon }
Expand All @@ -339,7 +340,7 @@ function GalleryEdit( props ) {
accept="image/*"
allowedTypes={ ALLOWED_MEDIA_TYPES }
multiple
value={ images }
value={ hasImageIds ? images : {} }
onError={ onUploadError }
notices={ hasImages ? undefined : noticeUI }
onFocus={ onFocus }
Expand Down