Skip to content
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.

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

Filters for Super Cool Ad Inserter Ads #839

Merged
merged 6 commits into from
Mar 24, 2020
Merged

Conversation

jeffersonrabb
Copy link
Contributor

@jeffersonrabb jeffersonrabb commented Mar 19, 2020

All Submissions:

Changes proposed in this Pull Request:

This branch is meant to be tested with Automattic/newspack-ads#31. Complete testing instructions can be found in Automattic/newspack-ads#31.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@jeffersonrabb jeffersonrabb marked this pull request as ready for review March 19, 2020 22:20
@jeffersonrabb jeffersonrabb changed the title feat: filters for super cool ad inserter placements Filters for Super Cool Ad Inserter Ads Mar 19, 2020
@jeffersonrabb jeffersonrabb self-assigned this Mar 19, 2020
@jeffersonrabb jeffersonrabb added the [Status] Needs Review The issue or pull request needs to be reviewed label Mar 19, 2020
@thomasguillot
Copy link
Contributor

This doesn't seem to work properly when the ads are inside a narrow content area (Default or One column). Wide content area (One column wide) seems to behave as expected.

As a temporary fix, I'd recommend that we remove the page template check and just use $breakpoint = ceil( $breakpoint / 0.9 );. So something like:

function newspack_theme_newspack_ads_breakpoint( $breakpoint, $placement, $context ) {
	// Apply Responsive placement to widgets using Super Cool Ad Inserter.
	$breakpoint = ceil( $breakpoint / 0.9 );

	return $breakpoint;
}
add_filter( 'newspack_ads_breakpoint', 'newspack_theme_newspack_ads_breakpoint', 10, 3 );

From here we can add some CSS to make sure that the ads are not overflowing.

E.g. in /sass/blocks/_blocks.scss

//! Super Cool Ad Inserter Plugin
.scaip {
	overflow: hidden;

	amp-ad {
		display: block;
	}
}

Then we will need to find a solution for the narrow content when viewport is >= 782px which means we should be using $breakpoint = ceil( $breakpoint / 0.585 );

Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

LGTM

@adekbadek adekbadek added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Mar 24, 2020
@jeffersonrabb jeffersonrabb merged commit e3ac56b into master Mar 24, 2020
@jeffersonrabb jeffersonrabb deleted the add/responsive-ads-scai branch March 24, 2020 17:01
matticbot pushed a commit that referenced this pull request Mar 24, 2020
# [1.3.0](v1.2.0...v1.3.0) (2020-03-24)

### Bug Fixes

* change way featured images are added to fix missing titles ([#837](#837)) ([7fb8ff2](7fb8ff2))
* correct position of comment icon indicating current author ([#824](#824)) ([6778719](6778719))
* prevent id selector format error ([#833](#833)) ([2fdb043](2fdb043))
* remove style overrides from h2 in cover blocks ([#834](#834)) ([15eb042](15eb042))
* remove top margin on accent-header in column ([#829](#829)) ([f72a529](f72a529))
* set max width for nested full and wide blocks ([#827](#827)) ([22e2655](22e2655))
* set size for above copyright widget titles ([#828](#828)) ([aa4231f](aa4231f))

### Features

* filters for super cool ad inserter placements ([#839](#839)) ([e3ac56b](e3ac56b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants