close
The Wayback Machine - https://web.archive.org/web/20221207193135/https://github.com/WordPress/performance/pull/578
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

Jump to bottom

Avoid potentially adding invalid attributes or duplicates for dominant color images #578

Merged
merged 15 commits into from Nov 17, 2022
Merged

Avoid potentially adding invalid attributes or duplicates for dominant color images #578

merged 15 commits into from Nov 17, 2022

Conversation

pbearne
Copy link
Contributor

@pbearne pbearne commented Nov 8, 2022 •
edited

Summary

Fixes #577

Relevant technical choices

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@pbearne pbearne requested a review from spacedmonkey as a code owner Nov 8, 2022
@pbearne pbearne linked an issue Nov 8, 2022 that may be closed by this pull request
Revise "Dominant Color" styling and inline styling #577
Closed
@pbearne pbearne added this to the 1.7.0 milestone Nov 8, 2022
@pbearne pbearne added the [Type] Bug An existing feature is broken label Nov 8, 2022
@mukeshpanchal27 mukeshpanchal27 added the [Focus] Images Issues related to the Images focus area label Nov 8, 2022
@bethanylang bethanylang requested a review from felixarntz Nov 8, 2022
@bethanylang bethanylang added this to Backlog in [Focus] Images via automation Nov 8, 2022
@bethanylang bethanylang moved this from Backlog to Review in [Focus] Images Nov 8, 2022
@bethanylang bethanylang added the Needs Review Anything that requires code review label Nov 8, 2022
felixarntz
felixarntz requested changes Nov 8, 2022
View changes
Copy link
Member

@felixarntz felixarntz left a comment

@pbearne While the approach for the fix makes sense to me, there are still a few flaws in the concrete logic.

One larger problem is that the logic now doesn't check whether the $style variable is not empty individually, which means that the else clause now can add a style="" - we don't want that.

See related comments below.

modules/images/dominant-color/load.php Outdated Show resolved Hide resolved
modules/images/dominant-color/load.php Outdated Show resolved Hide resolved
modules/images/dominant-color/load.php Show resolved Hide resolved
modules/images/dominant-color/load.php Outdated Show resolved Hide resolved
@felixarntz felixarntz changed the title add test for style before adding and add just CSS if found Avoid potentially adding duplicate or invalid style attributes for dominant color images Nov 8, 2022
pbearne and others added 4 commits Nov 9, 2022
@pbearne @felixarntz
Update modules/images/dominant-color/load.php
61879e2 
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@pbearne @felixarntz
Update modules/images/dominant-color/load.php
8859196 
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@pbearne
Merge branch 'trunk' into 577-revise-dominant-color-styling-and-inlin…
b3adffe 
…e-styling
@pbearne
Add check for src=" and bail if not found
eec7c37
@felixarntz felixarntz mentioned this pull request Nov 15, 2022
Closed
3 tasks
felixarntz
felixarntz reviewed Nov 15, 2022
View changes
Copy link
Member

@felixarntz felixarntz left a comment

@pbearne There is some overlap between this PR and #582.

Both of the PRs have something useful in them, but one part of #582 is a cleaner solution than the one here, while the PR here covers a few more problems. Therefore I added a suggestion below to limit your PR here to avoiding the addition of invalid attributes or duplicates from calling this function multiple times on the same image.

@felixarntz felixarntz changed the title Avoid potentially adding duplicate or invalid style attributes for dominant color images Avoid potentially adding invalid attributes or duplicates for dominant color images Nov 15, 2022
felixarntz
felixarntz reviewed Nov 15, 2022
View changes
Copy link
Member

@felixarntz felixarntz left a comment

@pbearne One more point of feedback here.

modules/images/dominant-color/load.php Outdated Show resolved Hide resolved
@felixarntz
Copy link
Member

felixarntz commented Nov 15, 2022

@pbearne FYI I just added that one change from the closed #573 into this PR in 77de817.

@felixarntz felixarntz assigned felixarntz and unassigned pbearne Nov 16, 2022
@felixarntz
Copy link
Member

felixarntz commented Nov 16, 2022

Will take over this one since it's a bug so we should get it merged today for the 1.7.0 plugin release.

@felixarntz
Copy link
Member

felixarntz commented Nov 16, 2022

@costdev I've made the relevant updates here, this should be ready for another review. Would be awesome if you could get to it today so we can merge it to get the release branch ready.

@felixarntz felixarntz requested a review from costdev Nov 16, 2022
@felixarntz felixarntz changed the base branch from trunk to release/1.7.0 Nov 16, 2022
@felixarntz
Copy link
Member

felixarntz commented Nov 16, 2022

Changed the base branch here to release/1.7.0 so that this will go into the 1.7.0 release once approved and merged.

@felixarntz felixarntz mentioned this pull request Nov 16, 2022
Closed
3 tasks
costdev
costdev approved these changes Nov 17, 2022
View changes
Copy link
Contributor

@costdev costdev left a comment

LGTM 👍

@mukeshpanchal27 mukeshpanchal27 merged commit a45e8ae into release/1.7.0 Nov 17, 2022
5 checks passed
[Focus] Images automation moved this from Review to Done Nov 17, 2022
@mukeshpanchal27 mukeshpanchal27 deleted the 577-revise-dominant-color-styling-and-inline-styling branch Nov 17, 2022
@felixarntz felixarntz mentioned this pull request Nov 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixarntz felixarntz

@mukeshpanchal27 mukeshpanchal27

@costdev costdev

@spacedmonkey spacedmonkey

Assignees

@felixarntz felixarntz

Labels
[Focus] Images Issues related to the Images focus area Needs Review Anything that requires code review [Type] Bug An existing feature is broken
Projects
[Focus] Images
Done
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.

Revise "Dominant Color" styling and inline styling
5 participants
@pbearne @felixarntz @mukeshpanchal27 @costdev @bethanylang