Skip to content

Commit

Permalink
Improve handling of dismiss cookie
Browse files Browse the repository at this point in the history
  • Loading branch information
@aricooperdavis
aricooperdavis committed Apr 29, 2021
1 parent ca277da commit 7979d39
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 14 deletions.
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
# v1.0.4
## 04/29/2021

1. [](#improved)
* Processing stops if dismiss cookie is set
* Dismiss cookie is unset on plugin config change

# v1.0.3
## 04/28/2021

1. [](#new)
* Added option to choose banner location (top or bottom)
2. [](#improved)
* Improved readme with Screenshots
* Improved config page with sections

# v1.0.2
## 04/28/2021
Expand Down
2 changes: 1 addition & 1 deletion blueprints.yaml
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: Custom Banner
slug: custom-banner
type: plugin
version: 1.0.3
version: 1.0.4
description: Add a custom banner to your Grav site
icon: bookmark
author:
Expand Down
16 changes: 14 additions & 2 deletions custom-banner.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@ public function autoload(): ClassLoader
*/
public function onPluginsInitialized(): void
{
// Don't proceed if we are in the admin plugin
// Do not show banner in admin
if ($this->isAdmin()) {
$this->enable([
'onAdminSave' => ['onAdminSave', 0],
]);
return;
}

Expand All @@ -66,6 +69,16 @@ public function onPluginsInitialized(): void
]);
}

public function onAdminSave(): void
{
// When updating plugin settings delete "dismiss" cookie
if ($this->grav['uri']->basename() == 'custom-banner') {
if (isset($_COOKIE[self::CUSTOM_BANNER_DISMISS])) {
setcookie(self::CUSTOM_BANNER_DISMISS, 'false', time()-1, $this->grav['uri']->rootUrl());
}
}
}

public function onAssetsInitialized(): void
{
$this->grav['assets']->addDirCss('plugins://custom-banner/css');
Expand All @@ -74,7 +87,6 @@ public function onAssetsInitialized(): void

public function onOutputGenerated(): void
{

// Get plugin config or fill with default if undefined
$config = $this->config();
$config['exclude-pages'] = (array)$config['exclude-pages'];
Expand Down
12 changes: 1 addition & 11 deletions js/custom-banner.js
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,4 @@
document.addEventListener('DOMContentLoaded', function() {
if (document.getElementsByClassName('custom-banner-dismiss')[0].style.display != 'none') {
if (document.cookie.split('; ').find(row => row.startsWith('custom-banner-dismiss'))) {
document.getElementsByClassName('custom-banner-container')[0].style.display = 'none';
}
} else {
document.cookie = "custom-banner-dismiss=false; max-age=0";
}
});

function custom_button_dismiss() {
document.cookie = 'custom-banner-dismiss=true; max-age=1800';
document.cookie = 'custom-banner-dismiss=true; max-age=1800;';
document.getElementsByClassName('custom-banner-container')[0].style.display = 'none';
}

5 comments on commit 7979d39

@pamtbaau
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind sharing what your thinking is behind the onAdminSave? I'm afraid I fail to see the benefits of it. I think the goal is to revive the banner when, for example, the text has changed. However, I suspect that that goal will not be achieved...

  • onAdminSaved() will only be called once when the Admin page has been saved.
  • It then sets a cookie with value false for the user logged in as Admin and it will expire immediately.
  • No front-end user will receive any change in its cookie, so the banner remains dismissed.

@aricooperdavis
Copy link
Owner Author

@aricooperdavis aricooperdavis commented on 7979d39 Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, as the cookie expiry time is only 30 mins I didn't think it was all that important for end-users to get the new banner straight away, but for the admin user making the changes it's useful to be able to see those changes straight away without having to either wait 30 mins or manually clear that cookie (which they won't necessarily know about).

So it doesn't solve the problem of displaying new banners to end users, but it solves a problem I'd run into as a developer trying to see changes I'd made to a banner that I'd dismissed!

@pamtbaau
Copy link
Contributor

@pamtbaau pamtbaau commented on 7979d39 Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK fair enough that the end-user doesn't get to see a changed banner right away.

What changes should the Admin be able to see straight away? The Admin doesn't get to see the banner anyway, isn't it?

@aricooperdavis
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well when I add a banner to the site the first thing I do is go to the site and see if I like the look of it and test that it works. If I make some changes I'll go back to the site and see if I like them.

However, you're right that there is an issue with visitors not seeing banner updates. How about:

  • The cookie value set by the visitor JS when they click the dismiss button is a hash of the HTML injected by the plugin
  • The server calculates this hash onAdminSave and checks it against the value in the visitors cookie onPluginInitialized rather than just checking that the cookie exists

@pamtbaau
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admin:
Hm... If I login as user and dismiss the banner and I login in to Admin in a new tab. I never get to see the changes in my 'user' tab... The cookie remains set. Probably because the paths in the cookie are different.

Visitor:
May I presume the server will save the hash calculated during onAdminSave?

  • The algorithm might work, but only when Admin is being used and not when the config file itself is being updated. You could instead use event onPageContentProcessed, which is fired when the cache is invalidated, which happens when any config file has changed, or when executing $ bin/grav cache.
  • I need more time to let the mind ponder over this...

Please sign in to comment.