Conversation
Co-authored-by: Crisoforo Gaspar Hernández <hello@crisoforo.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
westonruter
left a comment
There was a problem hiding this comment.
These suggestions should fix the tests now that the $context argument is required by wp_img_tag_add_decoding_async_attr().
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
# Conflicts: # src/wp-includes/media.php
9a2d84b to
79b931c
Compare
79b931c to
a6ddac5
Compare
felixarntz
left a comment
There was a problem hiding this comment.
@adamsilverstein This looks great. Just one thing that needs to be changed (decoding attribute has to be added before the wp_content_img_tag filter), plus two minor suggestions.
peterwilsoncc
left a comment
There was a problem hiding this comment.
My review is basically "what Felix said" with the addition of a change request in wp_get_attachment_image()
Thanks for your work implementing the new functions and extending the API, I really appreciate it.
felixarntz
left a comment
There was a problem hiding this comment.
@adamsilverstein Production code now lgtm, however note that PHPUnit tests around this are failing, e.g. https://github.com/WordPress/wordpress-develop/runs/6040566464?check_suite_focus=true.
Looks like that's mostly about the ordering of the attributes being added, so you probably just need to fix the tests.
| 'force_display' => false, | ||
| 'loading' => null, | ||
| 'extra_attr' => '', | ||
| 'decoding' => 'async', |
There was a problem hiding this comment.
Should this be added below 'loading' instead of below 'extra_attr'?
Confidence check me on this: As these are run through wp_parse_args(), this shouldn't introduce a BC break, right?
There was a problem hiding this comment.
Confidence check me on this: As these are run through
wp_parse_args(), this shouldn't introduce a BC break, right?
That's correct.
If a developer is defining their own decoding attribute in the function call, that will trump the one that is defined here.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
…evelop-fork into ticket/53232
Trac ticket: https://core.trac.wordpress.org/ticket/53232
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.