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

feat: "fixed height" a global setting with max threshold #590

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Jan 27, 2023

Changes how the "fixed height" system, implemented in #439, behaves. It's currently set as a per-placement setting and this PR turns it into a global setting with max height support:

image

If "max height" is enabled, any creative rendering above the defined threshold will cause a layout shift (CLS).

How to test

  1. Checkout this branch and fix(ads): remove fixed height setting newspack-plugin#2255
  2. Visit Advertising -> Settings and confirm you see the new "Fixed height" section with the above defaults
  3. Make sure you have GAM configured and set the Below Header and Footer placements with units containing the sizes: 728x90 970x90 970x250
  4. Visit the home page and confirm the CLS occurring from 100 to 250 if the slot renders the 970x250 creative
  5. Refresh until a creative with a height of 90px renders and confirm the slot height is at 100px without causing any CLS
  6. Change the "Maximum fixed height" value to 150 and confirm the value is applied on the front-end
  7. Disable the use of maximum fixed height, save, visit the front-end and confirm the slot is fixed with 250px of height and without any CLS
  8. Visit the Customizer, edit the placement, and confirm the "Fixed height" setting is gone and you can save without issues
  9. Visit "Advertising -> Placements", edit a placement, and also confirm the "Fixed height" setting is gone and you can save without issues
  10. Edit a page, add an Ad Unit block, confirm the "Fixed height" setting is gone and you can save without issues

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.

Left on wording suggestion, everything works as expected!

includes/class-fixed-height.php Outdated Show resolved Hide resolved
Co-authored-by: Adam Boro <aborowski24@gmail.com>
@miguelpeixe miguelpeixe merged commit 5f56f36 into master Jan 30, 2023
@miguelpeixe miguelpeixe deleted the feat/global-fixed-height branch January 30, 2023 13:28
@yogeshbeniwal
Copy link

May I know that issue this change will fix? from CLS point of view only Above Header and Below Header is the issue, that can be solved by enabling current feature i.e. Use fixed height for individual ads.

Setting Maximum fixed height globally as one size fit for all is not a good approach as different ad location has different better performing ad sizes.

@adekbadek
Copy link
Member

This change enforces an opinion that all ad slots should have defined fixed height, with a sensible maximum. This mitigates most of the CLS, but the feature can be tweaked to anyone's liking – e.g. if you want 0 CLS (from ads at least), set a very high "maximum fixed height". If you prefer to optimise for as little empty space around ads as possible, set a lower value or disable the feature.

Note that because the fixed-height feature will kick in for unfilled ads only for above-the-fold slots, this will in practice apply only to above- and below-header ads in most of the cases.

@yogeshbeniwal
Copy link

the fixed-height feature will kick in for unfilled ads only for above-the-fold slots

Thank you. This makes functionality clearer. Suggest to change descriptions to reflect 2 key things: unfilled ads + above-the-fold-slots only

matticbot pushed a commit that referenced this pull request Feb 17, 2023
# [1.42.0-alpha.1](v1.41.0...v1.42.0-alpha.1) (2023-02-17)

### Features

* "fixed height" a global setting with max threshold ([#590](#590)) ([5f56f36](5f56f36))
* **gam:** support "tag" key-val targeting their archive pages ([#595](#595)) ([3596c42](3596c42))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.42.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 28, 2023
# [1.42.0](v1.41.0...v1.42.0) (2023-02-28)

### Features

* "fixed height" a global setting with max threshold ([#590](#590)) ([5f56f36](5f56f36))
* **gam:** support "tag" key-val targeting their archive pages ([#595](#595)) ([3596c42](3596c42))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.42.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants