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

Enable cookie banner to set link styled as a button #2164

Merged
merged 3 commits into from
Mar 10, 2021

Conversation

hannalaakso
Copy link
Member

It should be possible to render a link styled as a button in the cookie banner. This is an edge case but could be useful for rendering the 'Hide' button if:

  • The cookie banner needs to work without JavaScript.
  • The user has already accepted/rejected cookies. This means that when the user presses 'Hide' and the page reloads, the banner will be hidden.

This PR:

  • Enables an action in the cookie banner to be a link styled as a button, with role="button" and draggable="false", if both href and type="button" are provided. The resulting markup matches Start buttons.
  • Add a test.
  • Rename another example that renders a link but also sets additional button attributes to test that they aren't rendered, to distinguish it from this new example.

We need to add some extra documentation to the macro options for this change. There are a few concepts mixed up in this change so I've started a doc for @EoinShaughnessy to help us finalise the wording.

Enable the action to be a link styled as a button, with role="button" and draggable="false", if both
href and type="button" are provided. This matches the [Start buttons](https://design-system.service.gov.uk/components/button/#start-buttons).

( A more terse solution to achieve this in the macro would be to skip the first condition with

```
{% if action.href && action.type != "button"% }
```

and pass a `href` to the button macro in `else`. This would work since Nunjucks doesn't render blank
attributes for options whose values aren't specified in the macro. However, other templating languages
might not follow a similar pattern - and we should try and keep our macro aligned with what ports of
GOV.UK Frontend do in their macros.)
Rename the example to distinguish it from the new link button example.
@hannalaakso hannalaakso added this to Needs review 🔍 in Design System Sprint Board via automation Mar 8, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2164 March 8, 2021 19:14 Inactive
@hannalaakso hannalaakso moved this from Needs review 🔍 to In progress 📝 in Design System Sprint Board Mar 8, 2021
@EoinShaughnessy
Copy link
Contributor

@hannalaakso:

  • To confirm: is this for a new parameter that we're adding to the 'Options for actions' table for Cookie banner macros?
  • Do we need a separate issue for the doc update?
  • Is it the cookie banner yaml here/the linked Google Doc that you want me to review? Or both?

@EoinShaughnessy
Copy link
Contributor

@hannalaakso Have commented on the Google Doc for the content change. :)

@36degrees 36degrees added this to the [NEXT] milestone Mar 10, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2164 March 10, 2021 17:38 Inactive
@hannalaakso
Copy link
Member Author

Merging this as @EoinShaughnessy has pulled the docs changes into #2168, including updates to the changelog.

@hannalaakso hannalaakso merged commit aafdd45 into master Mar 10, 2021
Design System Sprint Board automation moved this from Needs review 🔍 to Done 🏁 Mar 10, 2021
@hannalaakso hannalaakso deleted the enable-cookie-banner-to-set-link-as-button branch March 10, 2021 17:43
@hannalaakso hannalaakso moved this from Done 🏁 to Ready to release 🚀 in Design System Sprint Board Mar 10, 2021
@36degrees 36degrees mentioned this pull request May 13, 2021
@36degrees 36degrees moved this from Ready to release 🚀 to Done 🏁 in Design System Sprint Board May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants