Rails Girls 2015 SoC Banter


(Alan Tan) #86

Ah ok. This is how @eviltrout declared the plugin route.


(Ladydanger) #87

Hey @tgxworld!

First, thank you so much for taking the time to answer :slight_smile: the pretender library makes so much sense.

The question I have with the plugin route is that the purple-tentacle one navigates to plugins/purple-tentacle and it has it’s own route, whereas, our ad plugin doesn’t stipulate a route even though it is a plugin so I’m assuming it’s going to (/admin/plugins/discourse-adplugin).

What our plugin does do is that it uses client.yml to create two categories within site settings, and then uses inputs within site settings to inject inputs made by the user into a script.

en:
  admin_js:
    admin:
      site_settings:
        categories:
          ad_plugin: 'DFP Plugin'
          adsense_plugin: 'Adsense Plugin'

I did visit() the admin/plugin/discourse-adplugin route while playing around with qunit & it looked fine (as in it didn’t return any errors), but I’m confused because our inputs are in the /admin/site_settings/category/:id route and wanted to do fillIn() assertions. What are your thoughts @eviltrout?

Thanks again for looking into it @tgxworld, :ice_cream: really appreciate it! :smiley:

ps. rubybench looks awesome!!!


(Alan Tan) #88

Ah ok. I wasn’t sure about quite a number of things so I decided to dig deeper to understand it.

The reason why visiting /admin/site_settings doesn’t work is because there is nothing handling the following request.

In order to resolve that, we need pretender to be able to handle a call to /admin/site_settings on the “server” by adding a fixture.

Note how the first key in the map declares the URL that is going to be handled and the value is the data that should be return from the server. The fixtures are loaded here.

However…

The regexp we are using right now does not load a fixture that is in a plugin. So in order to make the above work, I actually added the file directly into the fixtures folder in Discourse.


(Ladydanger) #89

this makes it so easy to implement @tgxworld! Can’t wait to try it out today with my pair! Thanks again :+1: :crown:


(Kane York) #90

However, you’re testing the settings page more than your plugin in that case. Because you just plop in the response instead of having it generated from your site_settings.yml.


(Sarah Ni) #91

The officially endorsed Discourse Ad Plugin v0.1 is now available.
Install it now in 3 easy steps.
Includes support features for Google DFP and Adsense. :smile: :sunny:

Coming Soon: Support for Amazon Affiliates.


(Neil Lalonde) #92

Amazing work! :clap: I wrote that other old DFP plugin, and this one is looking like an upgrade.

Something a bit confusing is this setting:

adsense_show_topic_list_top: Disable ad at topic list top location

The setting says “show”, but the functionality is to disable? That checkbox is a bit confusing to me.

I’ll spend more time looking at the code later.


(Ladydanger) #93

Hi Neil,
That reminds me - we forgot to put in the credits for the plugins we referenced! Done now in the Readme! :slight_smile:

With the show to disable, we just negated the logic that so that people could put in the code and the ad would show, and if they wanted to disable it, they could tick the button.

Cheers,
Vi


(Neil Lalonde) #94

I reviewed the code a bit, and it looks good! :+1:

Some issues:

  • I tried it for DFP, but the ads don’t render because the width and height aren’t set when the component is rendered (code here). Looks like you handle this in the adsense and amazon components, so something similar is needed for DFP.

  • Code like this is confusing to read for the point I made earlier:

      if (settings.dfp_topic_list_top_code && !settings.dfp_show_topic_list_top)
    

It actually means “if the code is set and it is showing”, but it looks like “not showing”. Before too many people start using this plugin, that setting name should be changed to “dfp_disable_topic_list_top” or “dfp_hide_topic_list_top”. Using “show” to mean “hide” is confusing. :smile:


(Ladydanger) #95

Hi Neil!

Thanks so much for taking the time to review our code and for the feedback :smile: I’ve put it in our trello so we’ll definitely fix up the code with your useful suggestions when cyberkoi and I are in the office on Monday (we have this thing that we always pair in person :two_women_holding_hands:).

With DFP, is it showing a container or is there no container at all? The thing with DFP is that it’s quite slow to load on a new site even if it’s an existing ad (sometimes I leave it overnight) - I wrote a blog post about it - "How to see if you’ve put in your dfp gpt tag in properly. If an empty container’s showing, put in ?google_force_console and you can see if there’s an ad waiting to be served (and the ad should serve later on), but either way we’ll still have a look at this on Mondie! :wink: Havagreatweekend!


#96

Hope you can add a quick mockup design on where the ads show in the actual documentation so that we can reference which is the right ads we want to use :smile: great job on this guys checking it out now :smile:


#97

Also thus this work on a responsive Google Adsense? coz I tried using it still doesn’t show any ads I’m using a Google Adsense Responsive ads

UPDATE: this needs to be added in the ad format for responsive

data-ad-format="auto"

it will cater all add formats :smile:

here is the difference of the code in terms of the inline style ads vs responsive ads

responsive ads code

<script async src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js"></script>
<!-- Top Responsive Ads T&D Minimalist -->
<ins class="adsbygoogle"
     style="display:block"
     data-ad-client="{add code}"
     data-ad-slot="{add code}"
     data-ad-format="auto"></ins>
<script>
(adsbygoogle = window.adsbygoogle || []).push({});
</script>

720 x 90 ads code

<script async src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js"></script>
<!-- 728 x 90 T&D Modern White -->
<ins class="adsbygoogle"
     style="display:inline-block;width:728px;height:90px"
     data-ad-client="{add code}"
     data-ad-slot="{add code}"></ins>
<script>
(adsbygoogle = window.adsbygoogle || []).push({});
</script>

(Sarah Ni) #98

Hey @rnovino

Thanks for the feedback! Considering that Adsense does have size-specific and responsive-type ads, we should definitely get to supporting both options. Thanks for bringing this point up, it’s great to hear from users on what could be improved!

We do have screenshots on how ads would look in the README under Available Locations for Ad Display on the Image Location column. We’ll probably highlight it more since it might get overlooked. Hope the .jpg images will be useful! :smile:


#100

Not sure why ads are still not showing on my end :frowning: , is this a bug?


(Sarah Ni) #101

Heya @rnovino

  1. What were the inputs in the settings page that had resulted in this error?
  2. Try updating to the latest version of Discourse :smile:

#102

I just input my adsense id and ads id
using v1.4.0.beta12 +114


(Sarah Ni) #103

I replicated your settings page by inputting the ad code on every single desktop and mobile slot. It works perfectly! (Edit: I’m also using a similar local version - v1.4.0.beta12 +127)

Not sure what the issue is. Try choosing small numbers for adsense_nth_post_code, such as 1 or 2. Does it work now?


#104

Oh! I see didn’t notice that nth post I tought it’s still the ads code, but upon updating still ads doesn’t show on my end :frowning:

your master doesn’t have any .gitignore can that affect? since it’s still using some of your files?


UPDATE: It seems conflicting with the Curated Plugin so I disabled it for now and the Adsense worked :smile:


(Sarah Ni) #105

That’s strange. I wonder why it conflicted with your Curated Plugin.

We’ve updated the master to include .gitignore, amongst other minor changes.


(Alan Tan) #106

Are you sure the error came from the Adsense plugin instead of the Curated Plugin?

Try enabling the Curated plugin only and see if you still get the error. :smile: