Extended the plugin to manage ad layers for individual terms #56
Conversation
Extended the plugin to manage ad layers for terms
kala725
changed the title
|
Thanks for the PR @kala725! Before I give this a code review, it looks like this is only half the equation. The missing half would apply the ad layer if the term meta is present. Can you put that together? |
mboynes
added
needs unit tests
enhancement
|
Sure @mboynes. Will do the same. |
Adlayers, set layers when term specific ad layer is set
PHPCS error fixed
|
Hi @mboynes , I have updated the pull request as per the comments. Can you please have a look. |
|
@kala725 we asked for you to include unit tests as well, which still seem to be missing. That's why this hasn't been reviewed. Since you're introducing a new feature, test coverage is required before we'd consider merging it. Thanks. |
Test Cases for term_override of the ad_layer
|
@kala725 thanks for taking the time to include the unit tests. Upon further review, we're hesitant to merge in this feature because we believe it doesn't represent new functionality. Currently, you can create an ad layer and target it very specifically at term archive pages and specific term(s). Can you explain how this represents functionality that you can't currently achieve with the plugin? Thanks. |
|
@bcampeau : Suppose I want to target different kind of categories (Clothes, Electronics) with different ad-layers, there is no in-built functionality. Rather as per the current functionality, I can target Category archives to a single ad-layer. So like posts, If I need to target based on individual terms and not the complete taxonomy, It gives me a fine-grained control to do so. Hope the objective is clear. |
|
Hey @kala725, that is currently possible by adding the category/categories to the ad layer, just as you would add a category to a post. Give that a try and let us know how it goes! |
|
Hey @mboynes, that thing I have tried out , and that works the way. But As we have many custom taxonomies and terms associated with them will grow in multiples, It is harder to manage at ad-layers level, same as you have done for posts. So because of that purpose, kept that a separate entity same as posts. |
|
Thanks for testing that out and helping me hash out if this is beneficial or not.
Can you elaborate on why it's harder? This is where I'm undecided, so hearing more detail around that will be very helpful. Here's how I see it: let's say you had 1,000 terms with overrides. It seems to me that it would be more tenable to manage those within the ad layers instead of within the terms. If the terms are 1:1 with ad layers, you'd still have to create 1,000 ad layers, so it won't save you time/effort on that end. On the other hand, let's say the terms are 100:1 with ad layers (that is, you have 10 ad layers and 1000 terms) -- making changes to which terms use which layers would be significantly more efficient to manage in the ad layers, where you're managing it in 10 places instead of in 1,000 places. I'm still on the fence about merging this. We have to make absolutely certain that we don't add unnecessary functionality to the code. The more the plugin has/does, the more confusing it can be for users, and the harder it is to maintain in the long-run. |
|
@mboynes : Thanks Matthew, I will evaluate both the options once again and will get back to you soon. |
|
Thanks @kala725, we really appreciate your contribution to the plugin! |
@mboynes, Can you have a look at this ?