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

Fix: Image using an external source don't transform to a gallery #16548

Conversation

@jorgefilipecosta
Copy link
Member

commented Jul 11, 2019

This PR fixes a problem where when transforming images linking to an external URL into a gallery transformed the images into an empty gallery block.

The problem happened because the transform code ignored images without a media library id.

How has this been tested?

I pasted the following code in the code editor:

<!-- wp:image {"sizeSlug":"large"} -->
<figure class="wp-block-image size-large"><img src="https://via.placeholder.com/300.png?text=Image1" alt=""/></figure>
<!-- /wp:image -->

<!-- wp:image {"sizeSlug":"large"} -->
<figure class="wp-block-image size-large"><img src="https://via.placeholder.com/300.png?text=Image2" alt=""/></figure>
<!-- /wp:image -->

<!-- wp:image {"sizeSlug":"large"} -->
<figure class="wp-block-image size-large"><img src="https://via.placeholder.com/300.png?text=Image3" alt=""/></figure>
<!-- /wp:image -->

I selected all (and some in some tests) of the images and transformed them into a gallery.
I verified the resulting gallery works well in the editor and in the front end.
I checked the end to end tests passed.

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

@jorgefilipecosta One aspect of this that might be confusing for users is that after transforming to a gallery the Edit Gallery option doesn't include these images. What do you think?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

@talldan @jorgefilipecosta Now that we can add/remove and reorder images without opening the Media Gallery, maybe we should retire this "edit gallery" button in favor of the inline controls?
Needs design feedback and approval thought? @mapk @mtias

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

Now that we can add/remove and reorder images without opening the Media Gallery, maybe we should retire this "edit gallery" button in favor of the inline controls?

Hi @youknowriad, I feel the button to open the gallery modal is very useful for advanced users, e.g: it allows them to verify easily if the media objects contain the right details and allows to update them. While the block UI just updates the attributes of the blocks. Many users prefer to update the media object so when they use the same media else where they don't need to retype alts, captions etc.
Maybe we can now have a button in the inspector, to give less relevance to that option?

Anyway I guess we should not have the button to open media library when the images are external, one simply way is just hide the button if an external image exists.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

I just tested this and it appears to work as expected. Tranforming an Image block that uses an image from a URL to a Gallery does just that with no image loss.

image-url

I feel the button to open the gallery modal is very useful for advanced users, e.g: it allows them to verify easily if the media objects contain the right details and allows to update them.

I agree with this statement and also feel the button should remain for those reasons. But for images that are only linked by URL, then we can hide that item.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

I feel the button to open the gallery modal is very useful for advanced users,

I agree with this statement and also feel the button should remain for those reasons. But for images that are only linked by URL, then we can hide that item.

I still think that might not be a good assumption. We already had several complaints and user testing feedback indicating that editing gallery images is confusing and I think the button to edit all at one is the main problem for that. When you add a gallery, people often want to change a specific image (maybe by clicking on it or on a contextual button...). Might be good to get more feedback on that point though.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

One thing I like about the Edit Gallery option is that I can edit all the meta data for each image by using the Gallery as a gateway to get there.

Screen Shot 2019-07-25 at 2 39 17 PM

So my initial thought was to add these meta inputs in the Inspector for each image within the gallery when the image is selected.

BUT then I also noticed that a user can still edit the gallery without ever clicking the Edit button in the toolbar.

gallery

Basically, all this to say, because this option is still there, and seems like a secondary or tertiary action, I can see removing the edit button from the toolbar.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/converting-an-image-using-an-external-source-to-a-gallery-does-not-work branch from 82a12ff to accd6c1 Jul 26, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

I guess we should remove the edit gallery toolbar button in a different PR as it is not directly related to the transform bug. I created another PR that removes the button #16778. I guess both PR's can be merged in parallel.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/converting-an-image-using-an-external-source-to-a-gallery-does-not-work branch 2 times, most recently from 1d5eedb to 3f8eafc Aug 20, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

This PR was rebased and the related PR's were merged. It should be ready and just needs the final .

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

There's a failing test that should be easy to fix but otherwise LGTM

@jorgefilipecosta jorgefilipecosta force-pushed the fix/converting-an-image-using-an-external-source-to-a-gallery-does-not-work branch from 3f8eafc to fcce47d Aug 21, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the fix/converting-an-image-using-an-external-source-to-a-gallery-does-not-work branch 2 times, most recently from 3eec679 to c823c0d Aug 22, 2019
@jorgefilipecosta jorgefilipecosta merged commit 256a674 into master Aug 22, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/converting-an-image-using-an-external-source-to-a-gallery-does-not-work branch Aug 22, 2019
donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
dratwas added a commit to callstack/gutenberg that referenced this pull request Aug 28, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.