FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Avoid potentially adding invalid attributes or duplicates for dominant color images by pbearne · Pull Request #578 · WordPress/performance · GitHub

/ performance Public

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

Merged
mukeshpanchal27 merged 15 commits into
release/1.7.0from
577-revise-dominant-color-styling-and-inline-styling
Nov 17, 2022
ConversationCommits (15)ChecksFiles changed
Merged

Avoid potentially adding invalid attributes or duplicates for dominant color images#578
mukeshpanchal27 merged 15 commits into
release/1.7.0from
577-revise-dominant-color-styling-and-inline-styling

Conversation

pbearne commented Nov 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 requested a review from spacedmonkey as a code owner November 8, 2022 00:04
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 added this to the 1.7.0 milestone Nov 8, 2022
pbearne added the [Type] Bug An existing feature is broken label Nov 8, 2022
costdev reviewed Nov 8, 2022
View reviewed changes
Comment thread modules/images/dominant-color/load.php Outdated
costdev reviewed Nov 8, 2022
View reviewed changes
Comment thread modules/images/dominant-color/load.php Outdated
mxbclang requested a review from felixarntz November 8, 2022 13:56
felixarntz requested changes Nov 8, 2022
View reviewed changes

felixarntz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

Comment thread modules/images/dominant-color/load.php Outdated
Comment thread modules/images/dominant-color/load.php Outdated
Comment thread modules/images/dominant-color/load.php
Comment thread modules/images/dominant-color/load.php Outdated
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
costdev reviewed Nov 10, 2022
View reviewed changes
Comment thread modules/images/dominant-color/load.php Outdated
felixarntz mentioned this pull request Nov 15, 2022
Closed
3 tasks
felixarntz reviewed Nov 15, 2022
View reviewed changes

felixarntz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 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 reviewed Nov 15, 2022
View reviewed changes

felixarntz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@pbearne One more point of feedback here.

Comment thread modules/images/dominant-color/load.php Outdated

felixarntz commented Nov 15, 2022

Copy link
Copy Markdown
Member

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

felixarntz assigned felixarntz and unassigned pbearne Nov 16, 2022

felixarntz commented Nov 16, 2022

Copy link
Copy Markdown
Member

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 commented Nov 16, 2022

Copy link
Copy Markdown
Member

@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 requested a review from costdev November 16, 2022 19:16
felixarntz changed the base branch from trunk to release/1.7.0 November 16, 2022 20:42

felixarntz commented Nov 16, 2022

Copy link
Copy Markdown
Member

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 mentioned this pull request Nov 16, 2022
Closed
3 tasks
costdev approved these changes Nov 17, 2022
View reviewed changes

costdev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM 👍

mukeshpanchal27 merged commit a45e8ae into release/1.7.0 Nov 17, 2022
mukeshpanchal27 deleted the 577-revise-dominant-color-styling-and-inline-styling branch November 17, 2022 03:00
felixarntz mentioned this pull request Nov 17, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

felixarntz felixarntz approved these changes
mukeshpanchal27 mukeshpanchal27 approved these changes
spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner
+1 more reviewer
costdev costdev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

felixarntz

Labels

[Type] Bug An existing feature is broken

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

Revise "Dominant Color" styling and inline styling

5 participants

Back | FazBrowse Home | New Git URL