Additional template locations #64
Conversation
language/en/common.php
Outdated
| @@ -21,4 +21,6 @@ | |||
| $lang = array_merge($lang, array( | |||
| 'ADBLOCKER_TITLE' => 'Ad blocker detected', | |||
| 'ADBLOCKER_MESSAGE' => 'Our website is made possible by displaying online advertisements to our visitors. Please consider supporting us by disabling your ad blocker on our website.', | |||
|
|
|||
| 'POP_UP_TITLE' => 'ADVERTISEMENT', | |||
There was a problem hiding this comment.
Instead call this key ADVERTISEMENT. Then make the translation Advertisement (not all caps). This may come in handy in other places. We can make it uppercase with Twig in the template.
location/type/pop_up.php
Outdated
|
|
||
| $this->template->assign_vars(array( | ||
| 'POP_UP_COOKIE_NAME' => $this->config['cookie_name'] . '_pop_up', | ||
| 'POP_UP_COOKIE_EXPIRES' => gmdate('D, d M Y H:i:s T', time() + 86400), |
There was a problem hiding this comment.
I find this more readable:
gmdate('D, d M Y H:i:s T', strtotime('+1 day')),
location/type/pop_up.php
Outdated
| * @param \phpbb\language\language $language Language object | ||
| * @param \phpbb\request\request $request Request object | ||
| * @param \phpbb\config\config $config Config object | ||
| */ |
| @@ -0,0 +1,14 @@ | |||
| {% if AD_POP_UP %} | |||
There was a problem hiding this comment.
to be safe should probably also add AD_POP_UP_ID to this conditional
|
|
||
| $(window).on('load', function() { | ||
| document.cookie = '{{ POP_UP_COOKIE_NAME }}=true; expires={{ POP_UP_COOKIE_EXPIRES }}; path={{ POP_UP_COOKIE_PATH }}'; | ||
| phpbb.alert('{{ lang('POP_UP_TITLE') }}', `{% include '@phpbb_ads/phpbb_ads_default.html' %}`); |
There was a problem hiding this comment.
This is where you can easily uppercase this string like this:
{{ lang('POP_UP_TITLE')|upper }}
|
!set WIP |
| 'use strict'; | ||
|
|
||
| $(window).on('scroll', function() { | ||
| var page_footer = $('#page-footer'); |
There was a problem hiding this comment.
when storing an jquery object in a var it's a good practice to name those vars with $, ie: $page_footer
Also for JS lets use camelCase, ie: $pageFooter
| $(window).on('scroll', function() { | ||
| var page_footer = $('#page-footer'); | ||
| if (page_footer.offset().top - $(window).scrollTop() < $(window).height()) { | ||
| $('#phpbbad-slide-up').css({bottom: 0}); |
There was a problem hiding this comment.
Lets also add something that hides/slides the ad back down when the user scrolls back up from the bottom, so it goes away.
May be as simple as
if (page_footer.offset().top - $(window).scrollTop() > $(window).height()) {
$('#phpbbad-slide-up').css({bottom: '-1000px'});
}
There was a problem hiding this comment.
Something along these lines seems to work... could maybe use a little more finesse though:
$(window).on('scroll', function() {
var $pageFooter = $('#page-footer'),
$slider = $('#phpbbad-slide-up'),
position = $pageFooter.offset().top - $(window).scrollTop();
if (position < $(window).height()) {
$slider.css({bottom: 0});
}
if (position > $(window).height()) {
$slider.css({bottom: '-1000px'});
}
});There was a problem hiding this comment.
Looking for an effect sort of like this:
https://www.dropbox.com/s/abpavw2hjucydvx/banner_demo-sm.m4v?dl=0
There was a problem hiding this comment.
That video is with the code above ;)
styles/all/theme/phpbbads.css
Outdated
| #phpbbad-slide-up { | ||
| position: fixed; | ||
| right: 0; | ||
| bottom: -100%; |
There was a problem hiding this comment.
This should be a much bigger offset...in case of large ads..
There was a problem hiding this comment.
Oh just noticed it's 100%... Maybe go 200% lol ???
There was a problem hiding this comment.
Nope, we must find real solution. Otherwise animation will not work as expected.
styles/all/theme/phpbbads.css
Outdated
|
|
||
| #phpbbad-slide-up { | ||
| position: fixed; | ||
| right: 0; |
There was a problem hiding this comment.
We could probably remove this and just use width: 100%; for centering ads.
There was a problem hiding this comment.
@VSEphpbb to be honest, I didn't want this type of location to go that way. I always wanted to have it right aligned, shrank to the minimum space. I see your point now, as you posted a video. Right align won't look good on small screens. The only problem I have with 100% width is that content behind will be visible in some cases, but not clickable.
|
!unset WIP |
|
How would something not be clickable?
…Sent from my iPhone
On Aug 6, 2017, at 4:04 AM, Jakub Senko ***@***.***> wrote:
@senky commented on this pull request.
In styles/all/theme/phpbbads.css:
> @@ -2,3 +2,10 @@
display: flex;
justify-content: center;
}
+
+#phpbbad-slide-up {
+ position: fixed;
+ right: 0;
@VSEphpbb to be honest, I didn't want this type of location to go that way. I always wanted to have it right aligned, shrank to the minimum space. I see your point now, as you posted a video. Right align won't look good on small screens. The only problem I have with 100% width is that content behind will be visible in some cases, but not clickable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
| })(jQuery); | ||
| </script> | ||
| {% endif %} | ||
|
|
There was a problem hiding this comment.
Lets clean this up a little using the new includes folder and moving the actual ad code to their own dedicated files so this file may look more like:
{% if AD_POP_UP and AD_POP_UP_ID %}
{% set PHPBB_ADS_STYLE = '' %}
{% set PHPBB_ADS_CODE, PHPBB_ADS_ID = AD_POP_UP, AD_POP_UP_ID %}
{% include '@phpbb_ads/includes/phpbb_ads_pop_up.html' %}
{% endif %}
{% if AD_SLIDE_UP and AD_SLIDE_UP_ID %}
{% set PHPBB_ADS_STYLE = '' %}
{% set PHPBB_ADS_CODE, PHPBB_ADS_ID = AD_SLIDE_UP, AD_SLIDE_UP_ID %}
{% include '@phpbb_ads/includes/phpbb_ads_slide_up.html' %}
{% endif %}You'll probably need to merge the master branch into your branch here to get it up to date and have the new include folder.
| var $pageFooter = $('#page-footer'), | ||
| $slider = $('#phpbbad-slide-up'), | ||
| position = $pageFooter.offset().top - $(window).scrollTop(), | ||
| is_high_enough = $('body').height() > $(window).height() + $slider.height(); |
There was a problem hiding this comment.
This is causing some unextepted behavior. For example, I have one ad on apge that scrolls only a little, and the ad never appears because it's height is greater than the amount to scroll (due to + $slider.height()).
Also I notice that because you use $page-footer as part of the equation when a page loads that already displays some of the footer (but not all) you can either have 2 outcomes: Scroll to bottom reveals no ad...or there is enough scroll to reveal the ad, but scroll back up does not disappear the ad (because some of page-footer is still visible on the whole page).
Is that by design, because my expectation was that the ad would appear when you reach the bottom of a page scroll and imediately go away the instant you scroll back up a bit?
| @@ -0,0 +1,10 @@ | |||
| <script> | |||
| (function($) { | |||
There was a problem hiding this comment.
lets protect window and document, like:
(function($, window, document, undefined) {
....
})(jQuery, document, window);
| </div> | ||
|
|
||
| <script> | ||
| (function($) { |
There was a problem hiding this comment.
Also here, lets protect the window and document
| if ($(window).scrollTop() + $(window).height() >= $(document).height()) { | ||
| $slider.css({transform: 'translateY(0)'}); | ||
| } | ||
| else { |
There was a problem hiding this comment.
else should go on previous line, like: } else {
| var $slider = $('#phpbbad-slide-up'); | ||
|
|
||
| $(window).on('scroll', function() { | ||
| if ($(window).scrollTop() + $(window).height() >= $(document).height()) { |
There was a problem hiding this comment.
$(window) in here can technically be replaced with $(this)
There was a problem hiding this comment.
Yeah, but it's easier to read that way.
| } | ||
| }); | ||
|
|
||
| $('.phpbbad-slide-up-icon').on('click', function() { |
There was a problem hiding this comment.
I'd prefer a name more like phpbb-slide-up-close
| }); | ||
|
|
||
| $('.phpbbad-slide-up-icon').on('click', function() { | ||
| $slider.css({transform: 'translateY(100%)'}); |
There was a problem hiding this comment.
It might be nice to have dedicated functions for this: hideSlider and showSlider so that it makes perfect sens in their usages above which is showing it and which is hiding it. Or perhaps a jQuery function we create called $fn.toggleSlider() that accepts the args hide|show ??
| @@ -0,0 +1,25 @@ | |||
| <div id="phpbbad-slide-up"> | |||
| <div class="phpbbad-slide-up-hide"><i class="phpbbad-slide-up-icon icon fa-chevron-down"></i></div> | |||
There was a problem hiding this comment.
Lets add a title to the icon so a mouse-over says "Hide advertisement"
https://trello.com/c/M6zZtPzo/13-additional-template-locations
Also closes #8.