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

Disable the creation of srcset on SVGs #38

Closed
wants to merge 1 commit into from
Closed

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Feb 24, 2022

Description of the Change

In #23 we changed some behavior on how the height and width of SVGs are set. Previously those values were set to false, which in some instances just gets translated to zero. But this caused some issues in a few places, like the block editor, as technically those values are supposed to be integers and if those aren't converted to integers, it will cause PHP errors. We fixed this by using the actual height and width of the SVG and if those don't exist, we default to 100.

We've had a few reports though of where SVGs are now showing up larger than expected. I've only been able to reproduce this issue if directly using wp_get_attachment_image to output an SVG, though curiously I'm getting the wrong size images on the previous release as well. It seems the bug with the new release has to do with adding srcset to these images, whereas previously those weren't being added as we were overriding height and width to be false. I don't think we need srcset, so this PR removes that from all SVGs.

On the previous release, if using wp_get_attachment_image and not setting a size (so defaulting to thumbnail) I get this markup:

<img src="http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg" class="attachment-thumbnail size-thumbnail" alt="" loading="lazy" height="502.174" width="612">

This is incorrect as I should be getting an image that is 150x150, so I get a larger than expected image.

Using the same settings on the newest release:

<img width="150" height="150" src="http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg" class="attachment-thumbnail size-thumbnail" alt="" loading="lazy" srcset="http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg 150w, http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg 300w, http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg 1024w, http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg 1536w, http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg 2048w" sizes="(max-width: 150px) 100vw, 150px">

This gives me the right height and width set but because of the srcset, I can potentially get a larger or smaller image on various breakpoints.

After the changes in this PR:

<img width="150" height="150" src="http://oss.local/wp-content/uploads/2022/02/kiwi-2.svg" class="attachment-thumbnail size-thumbnail" alt="" loading="lazy">

Right height and width but no more srcset or sizes.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Fixed - Remove srcset and sizes from SVG image output

@dkotter dkotter self-assigned this Feb 24, 2022
@jeffpaul jeffpaul added this to the 2.0.0 milestone Feb 24, 2022
@dkotter
Copy link
Collaborator Author

dkotter commented Mar 11, 2022

This is now tackled in #44, so closing this out

@dkotter dkotter closed this Mar 11, 2022
@vikrampm1 vikrampm1 removed this from the 2.0.0 milestone May 25, 2022
@jeffpaul jeffpaul deleted the fix/srcset branch November 2, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants