Skip to content

Remove unnecessary leftover CSS for dominant color backgrounds on images#573

Closed
pbearne wants to merge 2 commits intotrunkfrom
bug/562
Closed

Remove unnecessary leftover CSS for dominant color backgrounds on images#573
pbearne wants to merge 2 commits intotrunkfrom
bug/562

Conversation

@pbearne
Copy link
Contributor

@pbearne pbearne commented Nov 2, 2022 •
edited by felixarntz
Loading

Summary

Fixes #562

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 added the [Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) label Nov 2, 2022
@pbearne pbearne added this to the 1.7.0 milestone Nov 2, 2022
@pbearne pbearne self-assigned this Nov 2, 2022
@pbearne pbearne requested a review from spacedmonkey as a code owner November 2, 2022 21:27
@mukeshpanchal27 mukeshpanchal27 added [Type] Bug An existing feature is broken [Focus] Images labels Nov 7, 2022
@costdev
Copy link
Contributor

costdev commented Nov 8, 2022

A recent commit resolves the missing class in the test failures. This should be resolved after rebasing this branch on trunk/merging trunk into this branch.

@mxbclang mxbclang requested a review from felixarntz November 8, 2022 13:57
felixarntz
felixarntz reviewed Nov 8, 2022
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.

@pbearne +1 to what @costdev says. This is good to approve once you've merged trunk back into this so that the unit tests should pass again.

@felixarntz felixarntz changed the title Remove not needed CSS left over from when we are try to use images Remove unnecessary CSS for dominant color backgrounds on images Nov 8, 2022
@felixarntz felixarntz changed the title Remove unnecessary CSS for dominant color backgrounds on images Remove unnecessary leftover CSS for dominant color backgrounds on images Nov 8, 2022
mukeshpanchal27
mukeshpanchal27 reviewed Nov 15, 2022
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

The users' complaint about the focal point is not addressed by this adjustment. The line in the code at https://github.com/WordPress/performance/blob/trunk/modules/images/dominant-color/load.php#L130 overrides any custom CSS that the focus point added, such as object-position: 45% 0%;.

@felixarntz
Copy link
Member

felixarntz commented Nov 15, 2022

@mukeshpanchal27 Good catch! I hadn't paid attention to what the concrete issue was about here.

@pbearne Per the above, and since between #578 and #582 that problem is actually fixed, I'll close this PR.

@felixarntz felixarntz closed this Nov 15, 2022
@felixarntz felixarntz removed this from the 1.7.0 milestone Nov 15, 2022
@felixarntz felixarntz mentioned this pull request Nov 15, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixarntz felixarntz felixarntz left review comments

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 left review comments

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

Assignees

@pbearne pbearne

Labels

[Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) [Type] Bug An existing feature is broken

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

With Dominant Color enabled, loses focal point settings

4 participants

@pbearne @costdev @felixarntz @mukeshpanchal27