From 29e6fa301d9fb187b62d7e7911d53f5401261a9d Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Fri, 27 Jan 2023 15:07:15 -0300 Subject: [PATCH 1/2] feat: "fixed height" a global setting with max threshold --- includes/class-core.php | 1 + includes/class-fixed-height.php | 71 +++++++++++++++++++ includes/class-placements.php | 12 +--- .../class-placement-customize-control.php | 40 ----------- .../class-broadstreet-provider.php | 3 +- includes/providers/gam/class-gam-model.php | 12 ++-- includes/providers/gam/class-gam-provider.php | 5 +- includes/providers/gam/class-gam-scripts.php | 29 ++++++-- src/blocks/ad-unit/index.js | 4 -- src/customizer/control.js | 3 - src/frontend/style.scss | 3 + src/placements/placement-control.js | 11 +-- 12 files changed, 108 insertions(+), 86 deletions(-) create mode 100644 includes/class-fixed-height.php diff --git a/includes/class-core.php b/includes/class-core.php index a008e835..d3cc1343 100644 --- a/includes/class-core.php +++ b/includes/class-core.php @@ -86,6 +86,7 @@ private function includes() { include_once NEWSPACK_ADS_ABSPATH . '/includes/providers/broadstreet/class-broadstreet-provider.php'; include_once NEWSPACK_ADS_ABSPATH . '/includes/class-settings.php'; include_once NEWSPACK_ADS_ABSPATH . '/includes/class-custom-label.php'; + include_once NEWSPACK_ADS_ABSPATH . '/includes/class-fixed-height.php'; include_once NEWSPACK_ADS_ABSPATH . '/includes/class-providers.php'; include_once NEWSPACK_ADS_ABSPATH . '/includes/class-bidding.php'; include_once NEWSPACK_ADS_ABSPATH . '/includes/bidders/class-medianet.php'; diff --git a/includes/class-fixed-height.php b/includes/class-fixed-height.php new file mode 100644 index 00000000..34cbf649 --- /dev/null +++ b/includes/class-fixed-height.php @@ -0,0 +1,71 @@ + __( 'Fixed height', 'newspack-ads' ), + 'help' => __( 'If enabled, ad slots will be rendered with a fixed height determined by the unit\'s sizes configuration.', 'newspack-ads' ), + 'section' => self::SECTION, + 'key' => 'active', + 'type' => 'boolean', + 'default' => true, + 'public' => true, + ], + [ + 'description' => __( 'Use maximum fixed height', 'newspack-ads' ), + 'help' => __( 'If enabled, rendered creatives larger than "maximum fixed height" will adjust the height accordingly and cause layout shift.', 'newspack-ads' ), + 'section' => self::SECTION, + 'key' => 'use_max_height', + 'type' => 'boolean', + 'default' => true, + 'public' => true, + ], + [ + 'description' => __( 'Maximum fixed height', 'newspack-ads' ), + 'help' => __( 'Maximum value for the fixed height applied to the ad slot, in pixels.', 'newspack-ads' ), + 'section' => self::SECTION, + 'key' => 'max_height', + 'type' => 'int', + 'default' => 100, + 'public' => true, + ], + ] + ); + } +} +Fixed_Height::init(); diff --git a/includes/class-placements.php b/includes/class-placements.php index 63868fed..1a10e97a 100644 --- a/includes/class-placements.php +++ b/includes/class-placements.php @@ -79,9 +79,6 @@ public static function register_api_endpoints() { 'bidders_ids' => [ 'sanitize_callback' => [ __CLASS__, 'sanitize_bidders_ids' ], ], - 'fixed_height' => [ - 'sanitize_callback' => 'rest_sanitize_boolean', - ], 'hooks' => [ 'sanitize_callback' => [ __CLASS__, 'sanitize_hooks_data' ], ], @@ -135,10 +132,6 @@ public static function sanitize_placement( $data ) { $sanitized_data['ad_unit'] = sanitize_text_field( $data['ad_unit'] ); } - if ( isset( $data['fixed_height'] ) ) { - $sanitized_data['fixed_height'] = rest_sanitize_boolean( $data['fixed_height'] ); - } - if ( isset( $data['bidders_ids'] ) ) { $sanitized_data['bidders_ids'] = self::sanitize_bidders_ids( $data['bidders_ids'] ); } @@ -235,7 +228,6 @@ public static function api_update_placement( $request ) { 'ad_unit' => $request['ad_unit'], 'bidders_ids' => $request['bidders_ids'], 'hooks' => $request['hooks'], - 'fixed_height' => $request['fixed_height'], 'stick_to_top' => $request['stick_to_top'], ]; $result = self::update_placement( $request['placement'], $data ); @@ -727,8 +719,6 @@ public static function inject_placement_ad( $placement_key, $hook_key = '' ) { $placement_data = $placement['data']; } - $placement_data['fixed_height'] = isset( $placement_data['fixed_height'] ) ? (bool) $placement_data['fixed_height'] : false; - if ( ! isset( $placement_data['ad_unit'] ) || empty( $placement_data['ad_unit'] ) ) { return; } @@ -765,7 +755,7 @@ public static function inject_placement_ad( $placement_key, $hook_key = '' ) { $placement_key . '-' . $hook_key => ! empty( $hook_key ), 'hook-' . $hook_key => ! empty( $hook_key ), 'stick-to-top' => $stick_to_top, - 'fixed-height' => $placement_data['fixed_height'], + 'fixed-height' => Settings::get_setting( 'fixed_height', 'active' ), ], $placement_key, $hook_key, diff --git a/includes/customizer/class-placement-customize-control.php b/includes/customizer/class-placement-customize-control.php index 8d4db966..037d079f 100644 --- a/includes/customizer/class-placement-customize-control.php +++ b/includes/customizer/class-placement-customize-control.php @@ -123,22 +123,6 @@ private function get_ad_unit_value( $hook_key = '' ) { return isset( $data['ad_unit'] ) ? $data['ad_unit'] : ''; } - /** - * Get the value of "fixed height" feature given its hook key. - * - * @param string $hook_key Optional hook key, will look root placement otherwise. - * - * @return string Ad unit ID or empty string if not found. - */ - private function get_fixed_height_value( $hook_key = '' ) { - $value = json_decode( $this->value(), true ); - $data = $value; - if ( ! empty( $hook_key ) && isset( $value['hooks'], $value['hooks'][ $hook_key ] ) ) { - $data = $value['hooks'][ $hook_key ]; - } - return isset( $data['fixed_height'] ) ? (bool) $data['fixed_height'] : false; - } - /** * Get the value of "stick to top" feature given its hook key. * @@ -278,29 +262,6 @@ private function render_bidder_input( $bidder_id, $value, $hook_key = '' ) { get_element_id( 'input', $id_args ); - $desc_id = $this->get_element_id( 'description', $id_args ); - ?> - - /> - - - - render_placement_hook_control( $hook_key, $this->placement ); } } - $this->render_fixed_height_checkbox( $this->get_fixed_height_value() ); if ( in_array( 'stick_to_top', $this->placement['supports'], true ) ) { $this->render_stick_to_top_checkbox( $this->get_stick_to_top_value() ); } diff --git a/includes/providers/broadstreet/class-broadstreet-provider.php b/includes/providers/broadstreet/class-broadstreet-provider.php index fcbfc689..197ec3ca 100644 --- a/includes/providers/broadstreet/class-broadstreet-provider.php +++ b/includes/providers/broadstreet/class-broadstreet-provider.php @@ -9,6 +9,7 @@ use Newspack_Ads\Providers\Provider; use Newspack_Ads\Core; +use Newspack_Ads\Settings; defined( 'ABSPATH' ) || exit; @@ -111,7 +112,7 @@ public function get_ad_code( $unit_id, $placement_key = '', $hook_key = '', $pla if ( ! self::is_plugin_active() || ! method_exists( '\Broadstreet_Utility', 'getZoneCode' ) ) { return ''; } - $fixed_height = isset( $placement_data['fixed_height'] ) ? (bool) $placement_data['fixed_height'] : false; + $fixed_height = Settings::get_setting( 'fixed_height', 'active' ); $attrs = []; $zones = $this->get_units(); $zone_idx = array_search( $unit_id, array_column( $zones, 'value' ) ); diff --git a/includes/providers/gam/class-gam-model.php b/includes/providers/gam/class-gam-model.php index adde23d4..c7c139f4 100644 --- a/includes/providers/gam/class-gam-model.php +++ b/includes/providers/gam/class-gam-model.php @@ -209,10 +209,9 @@ public static function get_ad_unit_for_display( $id, $config = array() ) { ); } - $unique_id = $config['unique_id'] ?? uniqid(); - $placement = $config['placement'] ?? ''; - $context = $config['context'] ?? ''; - $fixed_height = $config['fixed_height'] ?? false; + $unique_id = $config['unique_id'] ?? uniqid(); + $placement = $config['placement'] ?? ''; + $context = $config['context'] ?? ''; $ad_units = self::get_ad_units( false ); @@ -228,9 +227,8 @@ public static function get_ad_unit_for_display( $id, $config = array() ) { } $ad_unit = $ad_units[ $index ]; - $ad_unit['placement'] = $placement; - $ad_unit['context'] = $context; - $ad_unit['fixed_height'] = $fixed_height; + $ad_unit['placement'] = $placement; + $ad_unit['context'] = $context; $ad_unit['ad_code'] = self::get_ad_unit_code( $ad_unit, $unique_id ); $ad_unit['amp_ad_code'] = self::get_ad_unit_amp_code( $ad_unit, $unique_id ); diff --git a/includes/providers/gam/class-gam-provider.php b/includes/providers/gam/class-gam-provider.php index f0f204be..7164f3b7 100644 --- a/includes/providers/gam/class-gam-provider.php +++ b/includes/providers/gam/class-gam-provider.php @@ -83,9 +83,8 @@ public function get_ad_code( $unit_id, $placement_key = '', $hook_key = '', $pla $ad_unit = GAM_Model::get_ad_unit_for_display( $unit_id, array( - 'unique_id' => $placement_data['id'], - 'placement' => $placement_key, - 'fixed_height' => isset( $placement_data['fixed_height'] ) ? (bool) $placement_data['fixed_height'] : false, + 'unique_id' => $placement_data['id'], + 'placement' => $placement_key, ) ); if ( \is_wp_error( $ad_unit ) ) { diff --git a/includes/providers/gam/class-gam-scripts.php b/includes/providers/gam/class-gam-scripts.php index 7b30952f..c26b0eaa 100644 --- a/includes/providers/gam/class-gam-scripts.php +++ b/includes/providers/gam/class-gam-scripts.php @@ -66,6 +66,7 @@ public static function insert_gpt_footer_script() { } $network_code = GAM_Model::get_active_network_code(); + $fixed_height = Settings::get_settings( 'fixed_height' ); $prepared_unit_data = []; foreach ( GAM_Model::$slots as $unique_id => $ad_unit ) { @@ -110,7 +111,7 @@ public static function insert_gpt_footer_script() { 'code' => esc_attr( $ad_unit['code'] ), 'sizes' => $sizes, 'fluid' => (bool) $ad_unit['fluid'], - 'fixed_height' => (bool) $ad_unit['fixed_height'], + 'fixed_height' => $fixed_height, 'targeting' => $ad_targeting, 'sticky' => GAM_Model::is_sticky( $ad_unit ), 'size_map' => GAM_Model::get_ad_unit_size_map( $ad_unit ), @@ -250,8 +251,9 @@ function inOrPastViewport( element ) { * outside of the viewport will have 'auto' height. */ ?> - if ( ad_unit.fixed_height ) { + if ( ad_unit.fixed_height.active ) { var height = 'auto'; + var prop = 'height'; if ( ad_unit.in_viewport ) { for ( viewportWidth in ad_unit.size_map ) { if ( viewportWidth < availableWidth ) { @@ -261,9 +263,13 @@ function inOrPastViewport( element ) { } } } + if ( ad_unit.fixed_height.use_max_height && ad_unit.fixed_height.max_height < height ) { + height = ad_unit.fixed_height.max_height; + prop = 'min-height'; + } height = height + 'px'; } - container.parentNode.style.height = height; + container.parentNode.style[prop] = height; } } googletag.cmd.push(function() { @@ -357,7 +363,7 @@ function inOrPastViewport( element ) { * creatives to render on refresh control. */ ?> - if ( ad_unit.fixed_height && container.parentNode.style.height === 'auto' && event.size ) { + if ( ad_unit.fixed_height.active && container.parentNode.style.height === 'auto' && event.size ) { container.parentNode.style.height = event.size[1] + 'px'; event.slot.defineSizeMapping( googletag.sizeMapping().addSize( [ 0, 0 ], event.size ).build() ); } @@ -366,7 +372,7 @@ function inOrPastViewport( element ) { * Handle slot visibility. */ ?> - if ( event.isEmpty && ( ! ad_unit.fixed_height || ( ad_unit.fixed_height && ! ad_unit.in_viewport ) ) ) { + if ( event.isEmpty && ( ! ad_unit.fixed_height.active || ( ad_unit.fixed_height.active && ! ad_unit.in_viewport ) ) ) { container.parentNode.style.display = 'none'; } else { container.parentNode.style.display = 'flex'; @@ -408,10 +414,19 @@ public static function print_fixed_height_css( $placement_key, $hook_key, $place if ( 'gam' !== $placement_data['provider'] ) { return; } - if ( ! isset( $placement_data['fixed_height'] ) || ! $placement_data['fixed_height'] ) { + + $fixed_height = Settings::get_settings( 'fixed_height' ); + if ( ! $fixed_height['active'] ) { return; } + $get_height = function( $height ) use ( $fixed_height ) { + if ( $fixed_height['use_max_height'] && ! empty( $fixed_height['max_height'] ) ) { + $height = min( $height, $fixed_height['max_height'] ); + } + return sprintf( '%spx', $height ); + }; + $ad_units = GAM_Model::get_ad_units( false ); $ad_unit_idx = array_search( $placement_data['ad_unit'], array_column( $ad_units, 'id' ) ); if ( false === $ad_unit_idx ) { @@ -423,7 +438,7 @@ public static function print_fixed_height_css( $placement_key, $hook_key, $place echo ''; diff --git a/src/blocks/ad-unit/index.js b/src/blocks/ad-unit/index.js index 163b93a0..7905ec2a 100644 --- a/src/blocks/ad-unit/index.js +++ b/src/blocks/ad-unit/index.js @@ -42,10 +42,6 @@ export const settings = { type: 'object', default: {}, }, - fixed_height: { - type: 'boolean', - default: false, - }, // Legacy attribute. activeAd: { type: 'string', diff --git a/src/customizer/control.js b/src/customizer/control.js index ebcd6ddb..51edb452 100644 --- a/src/customizer/control.js +++ b/src/customizer/control.js @@ -37,9 +37,6 @@ import { set, debounce } from 'lodash'; $toggle.on( 'change', function () { updateValue( '', 'enabled', $( this ).is( ':checked' ) ); } ); - container.on( 'change', '.fixed-height-checkbox input[type=checkbox]', function () { - updateValue( '', 'fixed_height', $( this ).is( ':checked' ) ); - } ); container.on( 'change', '.stick-to-top-checkbox input[type=checkbox]', function () { updateValue( '', 'stick_to_top', $( this ).is( ':checked' ) ); } ); diff --git a/src/frontend/style.scss b/src/frontend/style.scss index 2cf43ece..e38da3ce 100644 --- a/src/frontend/style.scss +++ b/src/frontend/style.scss @@ -20,6 +20,9 @@ &.fixed-height { padding: 16px 0; box-sizing: content-box; + > * { + margin: 0; + } } } diff --git a/src/placements/placement-control.js b/src/placements/placement-control.js index ad3f06aa..36e63643 100644 --- a/src/placements/placement-control.js +++ b/src/placements/placement-control.js @@ -5,7 +5,7 @@ /** * WordPress dependencies */ -import { Notice, SelectControl, ToggleControl, TextControl } from '@wordpress/components'; +import { Notice, SelectControl, TextControl } from '@wordpress/components'; import { Fragment, useState, useEffect } from '@wordpress/element'; import { __, sprintf } from '@wordpress/i18n'; @@ -169,15 +169,6 @@ const PlacementControl = ( { } return null; } ) } - onChange( { ...value, fixed_height: data } ) } - /> ); }; From c4ad4cd54f3ce3a17881ebebf09f518325785f34 Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Mon, 30 Jan 2023 10:14:33 -0300 Subject: [PATCH 2/2] Update includes/class-fixed-height.php Co-authored-by: Adam Boro --- includes/class-fixed-height.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/class-fixed-height.php b/includes/class-fixed-height.php index 34cbf649..cb661789 100644 --- a/includes/class-fixed-height.php +++ b/includes/class-fixed-height.php @@ -47,7 +47,7 @@ public static function register_settings( $settings_list ) { 'public' => true, ], [ - 'description' => __( 'Use maximum fixed height', 'newspack-ads' ), + 'description' => __( 'Set a maximum fixed height', 'newspack-ads' ), 'help' => __( 'If enabled, rendered creatives larger than "maximum fixed height" will adjust the height accordingly and cause layout shift.', 'newspack-ads' ), 'section' => self::SECTION, 'key' => 'use_max_height',