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

Cookie banner component #108

Merged
merged 7 commits into from
Feb 17, 2021
Merged

Cookie banner component #108

merged 7 commits into from
Feb 17, 2021

Conversation

cpjmcquillan
Copy link
Collaborator

For some reason the component doesn't look quite so beautiful in the dummy app as it does in the design system for reasons unknown. If anybody has some ideas about why, give me a shout.

Also not sure on PR etiquette in this repo, but I've tried to follow patterns properly.

Let me know any thoughts you have!

Changes in this PR

  • Update govuk-frontend in the dummy app
  • Add GovukComponent::CookieBanner view component

Screenshots

image

@cpjmcquillan cpjmcquillan linked an issue Feb 17, 2021 that may be closed by this pull request
@peteryates
Copy link
Member

peteryates commented Feb 17, 2021

Awesome! Yeah, there are a couple of components that don't render perfectly on the dummy page, looks spot-on in an actual page though. Only minor thing left is to add it to the table in the README and we're good to go 👍🏽

@frankieroberto
Copy link
Collaborator

Could we try and follow the API conventions of the Nunjucks macro a bit more closely? Would make converting from prototypes to production code a bit easier.

For example, using heading instead of title and html instead of body?

Would also be good to support the hidden boolean too, as that might be needed by client-side implementations.

Copy link
Collaborator

@frankieroberto frankieroberto 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!

app/components/govuk_component/cookie_banner.html.erb Outdated Show resolved Hide resolved
@@ -0,0 +1,19 @@
<%= tag.div(class: classes, **html_attributes) do %>
<div class="govuk-cookie-banner__message govuk-width-container">
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be good to have a way to inject additional classes here...

app/components/govuk_component/cookie_banner.html.erb Outdated Show resolved Hide resolved
@peteryates
Copy link
Member

Thanks for the review @frankieroberto, I've made the title and actions optional and allowed for a customisable aria-label

For the time being I think we should stick with title and body as they're already used in various other components.

I'm not keen on the html param name used by the nunjucks repos as it's very general, but if there's a call for this library to be more in-line with them I'd be happy to migrate it that way at the point of a major release.

@peteryates
Copy link
Member

I just force pushed without the commit that allows for no actions. I think at least one action is necessary to be able to dismiss the banner. In the case of essential non-tracking cookies only I think not showing the banner is a more sensible move.

@peteryates peteryates merged commit e963f47 into master Feb 17, 2021
@peteryates peteryates deleted the cookie-banner-component branch February 17, 2021 12:35
@frankieroberto
Copy link
Collaborator

@peteryates thanks!

With all these ports of govuk-frontend there's a bit of a trade-off between making them more similar to the Nunjucks version vs making them more similar to the conventions of the language they're being ported into. It's a tricky balance!

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.

Cookie banner
3 participants