Two similar filters // may be needs refactoring #26

Closed
rinatkhaziev opened this Issue Apr 30, 2012 · 8 comments

3 participants

@rinatkhaziev

So we have acm_list_table_columns and acm_provider_columns. All the difference between them is that acm_list_table_columns return all table columns (e.g. id, name, priority, conditionals) and acm_provider_columns returns only ad network specific ones. May be we could use one filter and just skip service columns.

@danielbachhuber

Yep, I saw this as I was doing the final review and was somewhat confused as to the purpose of the second.

@rinatkhaziev

How about this?

https://gist.github.com/ed088bc8bd5a8a68cdee

If you have more elegant ideas I would be thrilled to hear 'em :)

@danielbachhuber

Bumping to v0.2.2

@rinatkhaziev

This is related to #34

@jeremyfelt jeremyfelt was assigned May 25, 2012
@danielbachhuber

Bumping to v0.2.3

@jeremyfelt

Is it necessary to have anything other than acm_list_table_columns? acm_provider_columns was only used in the provider definitions before, and only one provider can be used at a time, so it seems that it kind of works itself out.

Unless I'm missing something completely, which is entirely possible. :)

@rinatkhaziev

Well, given the nature of ad providers and that one platform could have many flavors, we need the ability to filter ad_code_args and then reflect it in WP_List_table columns. It would be nice to have one filter taking care of it, this might need some love: https://github.com/Automattic/Ad-Code-Manager/blob/develop/common/lib/acm-wp-list-table.php#L26

Make sense?

@danielbachhuber

I think my vote is to keep two filters, but have the values for 'acm_list_table_columns' based on 'acm_provider_columns' unless you explicitly decide to change which columns show. 'acm_provider_columns' could be better named though.

@rinatkhaziev rinatkhaziev added a commit that closed this issue Jun 26, 2012
@rinatkhaziev rinatkhaziev Closes #26: new configuration filter: acm_ad_code_args, readme update…
…s, make sure google-adsense conforms to new way
6933d2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment