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

Ensure we keep proper height and width settings for SVGs, unless those values don't exist #23

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Jan 17, 2022

Description of the Change

Previously we were setting the height and width of SVGs to be false, which can cause issues in certain places (like within Gutenberg) if they use those values for calculations. Instead, use the height and width values WordPress calculates for those files and if they don't exist for some reason, default those to 100, so we have a proper fallback value.

Closes #18

Alternate Designs

None

Possible Drawbacks

I'm unsure of why we were setting these to false to begin with. The function documentation even mentions we default to 100, but as far as I can tell, that was never happening. I've tested a number of various scenarios and I've not seen any issues with using the calculated height and width instead of using false but there's potential that I missed some scenario.

Verification Process

Follow the reproduction steps listed in #18 to verify the reported issue.

In addition, ensure that any SVG that gets uploaded displays correctly within the admin Media Library, within the attachment single page and when added to a Post or Page.

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 - Use calculated size for SVGs instead of using false

Credits

Darin Kotter added 2 commits January 17, 2022 10:52
… that WordPress has calculated. If no size exists, default the size to 100x100
@dkotter dkotter self-assigned this Jan 17, 2022
@jeffpaul jeffpaul requested a review from darylldoyle January 26, 2022 04:51
@jeffpaul jeffpaul added this to the 1.10.0 milestone Jan 26, 2022
Copy link
Collaborator

@darylldoyle darylldoyle left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.

I changed it from 100 to false in version 1.4.5. I feel it's because it was previously setting the width and height to 100px regardless of if the image had dimensions.

If I recall, the image sizes were always being passed as 0, so SVGs were always being rendered with a height and width of 0px. false is definitely incorrect though, so this is better than that.

I'd love to test whether this affects SVGs being rendered on the front end or not as we don't want to introduce a regression where SVGs render with 0px width/height.

@vralle vralle mentioned this pull request Feb 3, 2022
1 task
Copy link
Collaborator

@darylldoyle darylldoyle left a comment

Choose a reason for hiding this comment

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

Tested locally and looks good to me 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP 500 error in combination with Gutenberg plugin when website logo is an SVG file with custom dimensions
3 participants