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: Gallery does not link to full size images #16011

Merged

Conversation

@jorgefilipecosta
Copy link
Member

commented Jun 5, 2019

Description

Fixes: #13398

Currently, when we select a link Media File for the gallery, each image links to large size, that size is one already being seen by the user, the correct link to use should be the full-size image.
Classic editor galleries use the full-size image, and the image block too.
This PR makes sure full-size images are used.

We are changing the save and attributes logic but we don't need to add a deprecation because we are still compatibility with the previous gallery.
That compatibility is ensured because when "link to" is set to "media file" and the fullUrl attribute is not set we fall back to the image URL the value that was previously used.

How has this been tested?

I added some galleries I verified that when the "link to" option is set to "media file" the gallery links to full-size images.
I changed the galleries above to link to "attachment page" I saved and I reloaded the page, I changed "link to" to "media file" again. I verified the gallery still links to full-size images.
I created some galleries using Gutenberg master (including some galleries that link to "media file" ). I opened the post and verified there were no invalid blocks.

src={ image.url }
alt={ image.alt }
data-id={ image.id }
data-full-url={ image.fullUrl }

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 10, 2019

Member

Why should this be saved in the HTML?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 10, 2019

Author Member

I replicated exactly the same mechanism used in Attachment page links (data-link attribute), even if the user sets the link to none we save data-link attribute. This happens because the user may create a gallery set the link to none publish, and later come back to the editor and set the link to "Attachment page" or "Media File". When that happens, we need to have the full image URL, and the attachment page URL available so we can link to these destinations. It may also be useful for third-party plugins having these attributes available.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 11, 2019

Member

Why not set it as a block level attribute?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 26, 2019

Author Member

Hi @ellatrix the only way to set this attribute as a block-level attribute would be by using an attribute that is an array of strings containing the full URL of each image. But, then if the user reordered the images in code (which is something users may expect to work), it will break the block. We do something similar for image id's so the server parser can have access to attributes, but, even for this case we save a data-id attribute in HTML, and we have an attribute sync mechanisms to make sure the reordering works. Given that the server has access to image id's, it can quickly get the full URL, so there is no need to perform this mechanism another time.

I guess the ideal solution would be using nested image blocks so these attributes can be normal block attributes of the nested images. But, until we can implement that, I guess this solution is fine as it just replicates what we were doing for another attribute and solves a big problem affecting our users.

@@ -17,14 +17,23 @@ export default function save( { attributes } ) {

switch ( linkTo ) {
case 'media':
href = image.url;
href = image.fullUrl || image.url;

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 10, 2019

Member

In what cases is there no full size?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 10, 2019

Author Member

Current galleries don't have the fullUrl attribute set, this is a simple way to be back-compatible in these cases. Another even more important case where this may happen is when the gallery links to external images e.g: a set of image blocks linking to external URL's is transformed into a gallery.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

Thank you for the review and for the important questions asked @ellatrix 👍 I provided answers to both of them.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/gallery-does-not-link-to-full-size-images branch from 2b751f0 to 11f22aa Jul 10, 2019

@ChoccyHobNob

This comment has been minimized.

Copy link

commented Jul 23, 2019

Can someone PLEASE approve this!

@jorgefilipecosta jorgefilipecosta merged commit 0ec20b4 into master Aug 5, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/gallery-does-not-link-to-full-size-images branch Aug 5, 2019

@youknowriad youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.