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

Update notification banner to be flexible in what it accepts #119

Closed
fofr opened this issue Mar 10, 2021 · 5 comments · Fixed by #120
Closed

Update notification banner to be flexible in what it accepts #119

fofr opened this issue Mar 10, 2021 · 5 comments · Fixed by #120

Comments

@fofr
Copy link
Member

fofr commented Mar 10, 2021

The notification banner could include a variety of content. The API for this component should be as flexible as the design system's.

The following is not currently possible:

You are impersonating <a href="/">Paul Hayes</a> (email@email.com)

[Stop impersonating]

Suggestion:

  • Add an html param, to pass in and render any provided html

Like:

{{ govukNotificationBanner({
  html: html
}) }}

https://design-system.service.gov.uk/components/notification-banner/

This is affecting this PR: DFE-Digital/get-help-with-tech#1410

@peteryates
Copy link
Member

I think you should be able to do this now by passing in your content with a block:

<%= govuk_notification_banner(blah) do %>
  <h2>Inner content goes here</h2>
<% end %>

@fofr
Copy link
Member Author

fofr commented Mar 10, 2021

Ah, nice. Is there an example of this usage in the docs?

@peteryates
Copy link
Member

peteryates commented Mar 10, 2021

Not exactly, I'll try to get it updated at some point - the whole docs could do with a re-jig.

@asmega
Copy link
Contributor

asmega commented Mar 10, 2021

oh yea i remember now sorry. I think i had this problem before and now going thru the same hoops due to my poor memory. to do the above we subclassed the component and override def render? to always return true. in our case we can't use the given heading as it doesn't fit our format. but in order for the component to render it must have a heading hence why we override this

@peteryates
Copy link
Member

Ah! To be honest I think I added the render? method before thinking about allowing arbitrary content via blocks. I'll just remove it, can totally see why it's a pain in the rear 😅

peteryates added a commit that referenced this issue Mar 10, 2021
This allows the notification banner to be used in a more-flexible manner
but still prevents an empty one from being rendered.

Fixes #119
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 a pull request may close this issue.

3 participants