Skip to content

Fix image focal point bug when dominant color is enabled by not overriding style attribute #582

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
Merged
merged 7 commits into from
Nov 16, 2022
Merged

Fix image focal point bug when dominant color is enabled by not overriding style attribute #582

merged 7 commits into from
Nov 16, 2022

Conversation

@mukeshpanchal27
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 commented Nov 15, 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.

@mukeshpanchal27 mukeshpanchal27 added [Type] Bug An existing feature is broken [Focus] Images [Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) labels Nov 15, 2022
@mukeshpanchal27 mukeshpanchal27 added this to the 1.7.0 milestone Nov 15, 2022
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review November 15, 2022 10:36
mehulkaklotar
mehulkaklotar approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@mehulkaklotar mehulkaklotar left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 LGTM 🚀

@felixarntz felixarntz mentioned this pull request Nov 15, 2022
Closed
3 tasks
felixarntz
felixarntz reviewed Nov 15, 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.

@mukeshpanchal27 Production code implementation looks good, could you please add a unit test to cover that the change works correctly though?

@felixarntz felixarntz changed the title Fix the focal point problem when the dominant color is enabled Fix the focal point problem when the dominant color is enabled by not overwriting style attribute Nov 15, 2022
@felixarntz felixarntz changed the title Fix the focal point problem when the dominant color is enabled by not overwriting style attribute Fix the focal point problem when the dominant color is enabled by not overriding style attribute Nov 15, 2022
@felixarntz felixarntz changed the title Fix the focal point problem when the dominant color is enabled by not overriding style attribute Fix image focal point bug when the dominant color is enabled by not overriding style attribute Nov 15, 2022
@felixarntz felixarntz changed the title Fix image focal point bug when the dominant color is enabled by not overriding style attribute Fix image focal point bug when dominant color is enabled by not overriding style attribute Nov 15, 2022
@felixarntz felixarntz mentioned this pull request Nov 15, 2022
Merged
3 tasks
costdev
costdev reviewed Nov 16, 2022
View reviewed changes
mukeshpanchal27 and others added 2 commits November 16, 2022 15:00
@mukeshpanchal27 @costdev
Update test method name
3a77c75 
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
@mukeshpanchal27
Update test method description
057240e
costdev
costdev reviewed Nov 16, 2022
View reviewed changes
@mukeshpanchal27 @costdev
Added missing param
faff059 
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
costdev
costdev approved these changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback! LGTM now 👍

felixarntz
felixarntz approved these changes Nov 16, 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.

@mukeshpanchal27 Great, tests looks solid too!

@felixarntz felixarntz merged commit 3f7aae5 into trunk Nov 16, 2022
@felixarntz felixarntz deleted the fix/562-focal-point-issue branch November 16, 2022 18:57
@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

@pbearne pbearne pbearne approved these changes

@felixarntz felixarntz felixarntz approved these changes

@mehulkaklotar mehulkaklotar mehulkaklotar approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

+1 more reviewer

@costdev costdev costdev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mukeshpanchal27 mukeshpanchal27

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

1.7.0

Development

Successfully merging this pull request may close these issues.

With Dominant Color enabled, loses focal point settings

6 participants

@mukeshpanchal27 @pbearne @felixarntz @mehulkaklotar @costdev