Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow modules to start after cookie consent #2041

Merged
merged 1 commit into from
May 10, 2021

Conversation

andysellick
Copy link
Contributor

What

Allow cookie consent dependent modules to be initialised immediately when the user consents to cookies, so the page doesn't have to be reloaded first. This is done by creating an event listener for delayed modules, which is then triggered by the cookie banner, which continues module execution.

Full details of how this works are documented in an update to javascript-modules.md.

Why

We have a new feature on the bank holidays page where users can save their location, but this is done with cookies and so we don't show this feature unless cookies are accepted. Currently if you accept cookies on that page the feature doesn't appear until the page is reloaded.

This feature is an experiment, so it may not be continued, but I suspect that as we move towards a more personalised GOV.UK we will increasingly need the ability to initialise code like this on a page only after cookie consent is given.

Visual Changes

None.

Trello card: https://trello.com/c/QweAgiPc/693-bank-holidays-experiment-things-to-clean-up

@bevanloon bevanloon temporarily deployed to govuk-publis-create-del-hcilnx April 29, 2021 11:07 Inactive
@andysellick andysellick requested a review from alex-ju April 29, 2021 13:39
@andysellick andysellick force-pushed the create-delayed-modules branch from 42acbaf to 7b68f77 Compare April 29, 2021 14:18
@bevanloon bevanloon temporarily deployed to govuk-publis-create-del-hcilnx April 29, 2021 14:18 Inactive
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Thank you for persevering with this, Andy! My comment on the approach is briefly that we shouldn't need to alter the modules system or create delayed scripts. We can rely solely on events, as they naturally happen. The cookie banner emits user consent; a script listens to this event and carries on with setting up the script.

My reason for suggesting this approach is to prevent introducing complexity in the modules system which might be replaced in future if we move to a world where each component initialises itself when added to the DOM, potentially using the lifecycle callbacks.

@@ -88,6 +88,7 @@ window.GOVUK.Modules = window.GOVUK.Modules || {};
if (window.GOVUK.globalBarInit) {
window.GOVUK.globalBarInit.init()
}
window.GOVUK.modules.startDelayedModules()
Copy link
Contributor

Choose a reason for hiding this comment

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

Cookie banner could trigger the event itself (requires the if/else statement to make sure it works in IE, as you did in modules.js L97-L107, but I have simplified it below for illustrative purpose)

Suggested change
window.GOVUK.modules.startDelayedModules()
event = new window.CustomEvent('cookie-consent', { bubbles: true, cancelable: true })
element.dispatchEvent(event)

Then in the script (for now I get that we only need this for the Bank Holiday trial anyway)

SaveBankHolidayNation.prototype.start = function ($module) {
  this.$module = $module[0]
  if (consentCookie && consentCookie.settings) {
    this.setup()
  } else {
    window.addEventListener('cookie-consent', this.setup())
  }
}

SaveBankHolidayNation.prototype.setup() {
  // set things up to save location
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alex-ju that makes sense. I've rewritten it according to your suggestion. I was originally trying to make this be as flexible as possible and make sure that as little code was duplicated as possible by putting it into the modules code, but that's a good point, this is only for one thing and actually it's a lot cleaner this way.

Just need to rebase and check a few things and it should be ready for a re-review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, all done. Percy seems to have failed but I think it's still good to go.

@andysellick andysellick force-pushed the create-delayed-modules branch from ca941c8 to bf7bfdd Compare May 10, 2021 08:18
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Nice one!

@andysellick andysellick merged commit 70d5998 into master May 10, 2021
@andysellick andysellick deleted the create-delayed-modules branch May 10, 2021 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants