Skip to content

Ad Block: add fill color to Ad block icon.#13727

Merged
jeherve merged 1 commit intomasterfrom
fix/ad-block-icon-fill
Oct 28, 2019
Merged

Ad Block: add fill color to Ad block icon.#13727
jeherve merged 1 commit intomasterfrom
fix/ad-block-icon-fill

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Oct 11, 2019

Changes proposed in this Pull Request:

The block now looks like this:

screenshot 2019-10-11 at 18 35 51

screenshot 2019-10-11 at 18 34 25

![image](https://user-images.githubusercontent.com/426388/66668872-85582600-ec56-11e9-9dc5-09d38dc4c119.png)

Testing instructions:

  • Start with a site with a Jetpack Premium Plan
  • Add an ad block to your site, the icon should look good.
  • Switch to a dark theme like this one (dark mode can be activated from "Theme Options").
  • Open the editor again and check the look of the ad block.

Proposed changelog entry for your changes:

  • None

@jeherve jeherve added Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. [Block] Ad labels Oct 11, 2019
@jeherve jeherve added this to the 7.9 milestone Oct 11, 2019
@jeherve jeherve requested review from a team and simison October 11, 2019 16:40
@jeherve jeherve self-assigned this Oct 11, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Oct 11, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33937-code before merging this PR. Thank you!

@jeherve jeherve mentioned this pull request Oct 11, 2019
Merged
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Oct 11, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 7e5a4d9

simison
simison approved these changes Oct 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Haven't tested but looks good!

@ChaosExAnima ChaosExAnima added [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] Needs Review This PR is ready for review. labels Oct 23, 2019
@jeherve jeherve added [Status] Needs Review This PR is ready for review. [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Needs Review This PR is ready for review. labels Oct 28, 2019
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Oct 28, 2019

As discussed in Slack, it may be worth looking into using jpg instead of png here, as it may allow us to use slightly smaller images.

@simison
Copy link
Copy Markdown
Member

simison commented Oct 28, 2019

@jeherve did you mean to comment some other PR? :-) I'm not quite following...

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Oct 28, 2019

Oh yes, sorry 🤦‍♂

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Oct 28, 2019
kbrown9
kbrown9 approved these changes Oct 28, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

I tested this using both the light and dark modes of the suggested theme. Both look good!

@kbrown9 kbrown9 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 28, 2019
@jeherve jeherve merged commit 5e761ba into master Oct 28, 2019
@jeherve jeherve deleted the fix/ad-block-icon-fill branch October 28, 2019 21:17
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simison simison simison approved these changes

@kbrown9 kbrown9 kbrown9 approved these changes

Assignees

@jeherve jeherve

Labels

[Block] Ad Bug When a feature is broken and / or not performing as intended Touches WP.com Files

Projects

None yet

Milestone

7.9

Development

Successfully merging this pull request may close these issues.

6 participants

@jeherve @matticbot @jetpackbot @simison @kbrown9 @ChaosExAnima