search

alleyinteractive / ad-layers

Ad Layers WordPress Plugin
Other
26 stars 12 forks source link

Try to set a default ad layer if no ad layer has been defined. #71

Closed lovasco-j closed 3 years ago

lovasco-j commented 6 years ago

This PR is to set a default adlayer if no adlayer has been defined in Ad_Layers set_active_ad_layer. This would allow a default layer to be used on a variety of pages without having to set granular layers such as the author or 404 pages.

mboynes commented 6 years ago

Thanks for the PR @lovasco-j! I'm interested to also get @bcampeau's two cents.

I can be convinced that this is the right approach to solve this problem, though my one reservation is that "default" has a purpose: to catch the page types not listed (in retrospect, maybe we should have named it "other pages" or something more descriptive). So if you want to differentiate between a page type not listed (what is currently "default") and the fallback as is proposed in this PR, you can't. Playing devil's advocate against myself, "default" sounds like it should work as this PR is proposing.

Here are the two paths forward that I see:

  1. Accept this PR as-is and stop over-thinking this
  2. Introduce a new page type "Other pages" and make that work how "Default" works now, then make "Default" work how this PR proposes
    • This should preserve backwards compatibility even though we'd be changing the meaning of "Default", since the new "Default" would catch the pages that layers should assign to "Other pages".
lovasco-j commented 6 years ago

@mboynes thanks for looking this over. Currently the is_singular() check online 227 is absorbing a lot of the other pages if there is an adlayer with no requirements assigned to it. This was a bit confusing to me because I wasn't sure where the Default page type was explicitly called out in set_active_ad_layer; turns out it is not. This PR would also give all of the predefined page types an expected result in set_active_ad_layer. @bcampeau what are your thoughts?