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

Update how custom svg dimesions are applied #216

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

gabriel-glo
Copy link

@gabriel-glo gabriel-glo commented Jul 22, 2024

Description of the Change

  • Update how custom svg dimesions are applied for get_image_tag and wp_get_attachment_image functions
  • Separate the logic for defining custom dimensions in a set_svg_dimension function

Closes #6

How to test the Change

In any template output the image markup either by using get_image_tag or wp_get_attachment_image functions.
Set the custom image size to either one of the registered image size names or a custom one via array [$width, $height].
Output should match the set dimensions.

Changelog Entry

Added - added set_svg_dimension function in order to reduce repetition
Changed - changed how image dimensions were passed in get_image_tag_override and one_pixel_fix methods

Credits

Props @

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

Update how custom svg dimesions are applied for `get_image_tag` and `wp_get_attachment_image` functions
Separate the logic for defining custom dimensions in a `set_svg_dimension` function
@gabriel-glo gabriel-glo self-assigned this Jul 22, 2024
@gabriel-glo gabriel-glo linked an issue Jul 22, 2024 that may be closed by this pull request
@github-actions github-actions bot added this to the Future Release milestone Jul 22, 2024
@github-actions github-actions bot added the needs:code-review This requires code review. label Jul 22, 2024
@github-actions github-actions bot removed the needs:code-review This requires code review. label Jul 22, 2024
Copy link

@gabriel-glo thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this?

@github-actions github-actions bot added the needs:feedback This requires feedback to determine next steps. label Jul 22, 2024
@jeffpaul jeffpaul requested review from iamdharmesh and removed request for dkotter and jeffpaul August 14, 2024 01:51
iamdharmesh
iamdharmesh previously approved these changes Aug 24, 2024
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR @gabriel-glo. Code looks good and it tests well. (Added one minor doc update suggestion)

@dkotter The PR looks good to me. However, since we had 2-3 size/dimension-related PRs on this plugin earlier and you have better context on those, I wanted to have a quick review from your side to ensure these changes align with those PRs.

Thank you.

safe-svg.php Outdated Show resolved Hide resolved
@jeffpaul jeffpaul modified the milestones: Future Release, 2.3.0 Aug 26, 2024
safe-svg.php Outdated
if ( is_array( $size ) ) {
$width = $size[0];
$height = $size[1];
} elseif ( 'full' === $size && $dimensions ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the output of svg_dimensions can be filtered, I think we'll need to be more defensive in a few places here to ensure 1 - $dimensions is an array and 2 - it contains the height and width keys

safe-svg.php Outdated
*
* @return int|false Width or height of the SVG image, or false if not found.
*/
protected function set_svg_dimensions( $id, $size ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice to add unit tests to cover this new method

safe-svg.php Outdated
@@ -728,6 +717,37 @@ protected function str_ends_with( $haystack, $needle ) {
return 0 === substr_compare( $haystack, $needle, -$len, $len );
}

/**
* Set custom width or height of the SVG image.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this method doesn't actually set anything, it returns these values, we should probably update this

Copy link
Collaborator

Choose a reason for hiding this comment

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

And thinking about it more, might be worth renaming this method to be get_svg_dimensions instead, or something similar

safe-svg.php Outdated Show resolved Hide resolved
Co-authored-by: Darin Kotter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:feedback This requires feedback to determine next steps.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVG height and width attributes
4 participants