Skip to content

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

Merged
mukeshpanchal27 merged 15 commits intorelease/1.7.0from
577-revise-dominant-color-styling-and-inline-styling
Nov 17, 2022
Merged

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

Conversation

@pbearne
Copy link
Contributor

@pbearne pbearne commented Nov 8, 2022 •
edited
Loading

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 November 8, 2022 00:04
@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
costdev
costdev reviewed Nov 8, 2022
View reviewed changes
costdev
costdev reviewed Nov 8, 2022
View reviewed changes
@mxbclang mxbclang requested a review from felixarntz November 8, 2022 13:56
felixarntz
felixarntz requested changes 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 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.

@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
costdev
costdev reviewed Nov 10, 2022
View reviewed changes
@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.

@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 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 One more point of feedback here.

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

LGTM 👍

@mukeshpanchal27 mukeshpanchal27 merged commit a45e8ae into release/1.7.0 Nov 17, 2022
@mukeshpanchal27 mukeshpanchal27 deleted the 577-revise-dominant-color-styling-and-inline-styling branch November 17, 2022 03:00
@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 felixarntz approved these changes

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 approved these changes

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

+1 more reviewer

@costdev costdev costdev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

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

@pbearne @felixarntz @mukeshpanchal27 @costdev @mxbclang