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.

Sign up for GitHub

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

Jump to bottom

Update auto sizes logic in Enhanced Responsive Images plugin to no longer load if already in Core #1547

Merged
merged 6 commits into from
Sep 23, 2024
Merged

Update auto sizes logic in Enhanced Responsive Images plugin to no longer load if already in Core #1547

merged 6 commits into from
Sep 23, 2024

Conversation

mukeshpanchal27
Copy link
Member

Summary

Fixes #1531

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature no milestone PRs that do not have a defined milestone for release [Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) skip changelog PRs that should not be mentioned in changelogs labels Sep 19, 2024
@mukeshpanchal27 mukeshpanchal27 self-assigned this Sep 19, 2024
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review September 19, 2024 04:53
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 19, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter westonruter removed the no milestone PRs that do not have a defined milestone for release label Sep 20, 2024
@westonruter westonruter added this to the auto-sizes 1.3.0 milestone Sep 20, 2024
westonruter
westonruter approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM, with a minor suggestion.

@westonruter westonruter removed the skip changelog PRs that should not be mentioned in changelogs label Sep 20, 2024
joemcgill
joemcgill approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

One suggestion, but this will get the job done.

plugins/auto-sizes/tests/test-auto-sizes.php Outdated Show resolved Hide resolved
felixarntz
felixarntz approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 This looks good. While the test is not super important to have (as it tests something very simple), technically it shouldn't be removed as it's still relevant. See feedback below.

plugins/auto-sizes/hooks.php Outdated Show resolved Hide resolved
plugins/auto-sizes/tests/test-auto-sizes.php Outdated
Comment on lines 35 to 39
public function test_hooks(): void {
$this->assertSame( 10, has_filter( 'wp_get_attachment_image_attributes', 'auto_sizes_update_image_attributes' ) );
$this->assertSame( 10, has_filter( 'wp_content_img_tag', 'auto_sizes_update_content_img_tag' ) );
$this->assertSame( 10, has_action( 'wp_head', 'auto_sizes_render_generator' ) );
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit-pick, but technically it would be more appropriate to keep this test but make it behave differently based on whether the Core function exists. If it does not exist, the same assertions apply like now. If it does exist, you could assert that the first two filters are not added.

Note also that the generator filter is relevant either way, so that's another reason not to remove this test.

Copy link
Member

Choose a reason for hiding this comment

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

As I suggested in #1547 (comment) correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Re-added in de6022c

mukeshpanchal27 and others added 2 commits September 23, 2024 20:48
@mukeshpanchal27 @felixarntz
Apply suggestions from code review
fb5d2ff 
Co-authored-by: Felix Arntz <felixarntz@google.com>
@mukeshpanchal27
Re-add test_hooks test
de6022c
felixarntz
felixarntz approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @mukeshpanchal27!

plugins/auto-sizes/tests/test-auto-sizes.php Outdated Show resolved Hide resolved
@westonruter westonruter merged commit d86cf87 into trunk Sep 23, 2024
15 checks passed
@westonruter westonruter deleted the fix/1531-update-auto-sizes-logic branch September 23, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixarntz felixarntz felixarntz approved these changes

@westonruter westonruter westonruter approved these changes

@joemcgill joemcgill Awaiting requested review from joemcgill joemcgill is a code owner

Assignees

@mukeshpanchal27 mukeshpanchal27

Labels
[Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Milestone
auto-sizes 1.3.0
Development

Successfully merging this pull request may close these issues.

Update auto sizes logic in Enhanced Responsive Images plugin to no longer load if already in Core
4 participants
@mukeshpanchal27 @westonruter @joemcgill @felixarntz