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

WIP: Add cookie banner #227

Closed
wants to merge 5 commits into from
Closed

WIP: Add cookie banner #227

wants to merge 5 commits into from

Conversation

kr8n3r
Copy link
Contributor

@kr8n3r kr8n3r commented May 5, 2021

What

Implement a configurable cookie banner based on the GOV.UK Design system cookie banner component

How to test

To do:

@kr8n3r kr8n3r force-pushed the add-cookie-banner branch 3 times, most recently from 1b984b1 to 6256243 Compare May 8, 2021 09:33
@kr8n3r kr8n3r added the enhancement New feature or request label May 10, 2021
@kr8n3r kr8n3r force-pushed the add-cookie-banner branch 4 times, most recently from 81d8885 to a650de3 Compare May 10, 2021 15:53
Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, Jani. Just got a couple of comments, hope they make sense!

I also noticed when running this locally and looking at the Design System frontend docs site that URLs seem to link directly to the first heading on a page, which means the cookie banner is automatically scrolled out of view. I'm not sure where that behaviour comes from, but I wonder what the reason was behind it and if it's something teams could turn off in the config (otherwise it's unlikely anyone will see the banner) ?

lib/source/layouts/_cookie_banner.erb Outdated Show resolved Hide resolved
lib/source/layouts/_cookie_banner.erb Outdated Show resolved Hide resolved
lib/source/layouts/_cookie_banner.erb Outdated Show resolved Hide resolved
@kr8n3r
Copy link
Contributor Author

kr8n3r commented May 11, 2021

Thanks for working on this, Jani. Just got a couple of comments, hope they make sense!

I also noticed when running this locally and looking at the Design System frontend docs site that URLs seem to link directly to the first heading on a page, which means the cookie banner is automatically scrolled out of view. I'm not sure where that behaviour comes from, but I wonder what the reason was behind it and if it's something teams could turn off in the config (otherwise it's unlikely anyone will see the banner) ?

it looks like it's driven by in-page-navigation.js I don't think there's a config setting for this

https://docs.cloud.service.gov.uk/ imports a markdown file for the first page and that somehow circumvents the JS

@kr8n3r kr8n3r force-pushed the add-cookie-banner branch 3 times, most recently from 5a1c879 to 8070c4d Compare May 14, 2021 09:57
@kr8n3r
Copy link
Contributor Author

kr8n3r commented May 14, 2021

Build is failing. See #230 for detail Fixed in #233

@kr8n3r kr8n3r force-pushed the add-cookie-banner branch 2 times, most recently from 6fe2f5d to 545d4c3 Compare May 18, 2021 08:34
@m-green
Copy link
Contributor

m-green commented Oct 14, 2021

Hi @kr8n3r - thanks for working on this! Was it ready for another review or did you need to do more work on it? (I've added @lfdebrux as a reviewer for when it's ready.)

@jonathanglassman Are you ok to work with @kr8n3r on the related documentation PR? I'm happy to pre-i or 2i it.

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Oct 21, 2021

Hi @kr8n3r - thanks for working on this! Was it ready for another review or did you need to do more work on it? (I've added @lfdebrux as a reviewer for when it's ready.)

@jonathanglassman Are you ok to work with @kr8n3r on the related documentation PR? I'm happy to pre-i or 2i it.

it does require writing tests for it, but that would mean setting up a new test framework.
I worked on this when I had some time (which was a while ago) but it's not of any priority for us at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

3 participants