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

Halloween's over - let's stamp out these ZOMBIE merchandising components #11055

Merged
merged 2 commits into from
Nov 5, 2015

Conversation

jbreckmckye
Copy link
Contributor

This is a fix to a slightly obscure spacefinder bug.

Sometimes, inline merchandising components are added in spaces they really don't belong:

image

What's happening here? Aren't these things supposed to give headings clearance, because they're left aligned?

The rules they use certainly say so...

lenientRules.selectors[' > h2'].minAbove = 20;

So is this just spacefinder on the fritz? Let's take a look at the comparison it does between the elements for the purposes of the inlineMercPromise...

image

Both isMinAbove and isMinBelow are false for this check. So spacefinder considers the paragraph in question ineligible for the inline merchandising component. Yet it displays there all the same. What gives?

A look at article-body-adverts.js gives the answer...

if (config.page.hasInlineMerchandise) {
    adNames.unshift(['im', 'im']);

    inlineMercPromise = spacefinder.getParaWithSpace(lenientRules).then(function (space) {
        return insertAdAtP(space);
    });
} else {
   inlineMercPromise = Promise.resolve(null);
}

We're adding the inline merchandising ad names - ['im', 'im'] - regardless of whether the spacefinder actually finds a space for the IM component. So the next time we do add an advert, it'll be an IM component instead, even though there's no space for such a thing. Thus I have termed it a zombie merchandising component.

Originally, this wouldn't have been a problem, because the IM element was added with lenientRules - how would we later find ad spaces with stricter rules? But the lenientRules have been subtly changed at some point - they now have a h2 clearance that wouldn't otherwise be in place on desktop - and are now not quite as lenient after all.

The fix is to -

  • only add the 'im' adnames when we're actually able to inject an adslot, and
  • rename lenientRules because their name is misleading

And now the zombie is gone, with the proper inline MPU in its place -

image

@paperboyo
Copy link
Contributor

This is gonna be totally off-topic, but I just wanted to express sheer amazement at the way you thoroughly explain your PRs, @jbreckmckye! It's like having -verbose added to your username every time... One learns so much from you, thanks (or one could, would one understood a thing)!

@jbreckmckye
Copy link
Contributor Author

@paperboyo - That is very kind of you to say! I'd always worried whether I was being a bit too verbose.

@uplne
Copy link
Contributor

uplne commented Nov 4, 2015

👍
zombie

jbreckmckye added a commit that referenced this pull request Nov 5, 2015
Halloween's over - let's stamp out these ZOMBIE merchandising components
@jbreckmckye jbreckmckye merged commit ceec683 into master Nov 5, 2015
@jbreckmckye jbreckmckye deleted the jbmk-zombie-inline-merchandising branch November 5, 2015 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants