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

Add notifications banner template macro and tests #1977

Merged
merged 5 commits into from
Oct 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/govuk/components/notification-banner/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Notification banner

## Installation

See the [main README quick start guide](https://github.com/alphagov/govuk-frontend#quick-start) for how to install this component.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably update these to point to the Frontend docs at some point…


## Guidance and Examples

Find out when to use the notification banner component in your service in the [GOV.UK Design System](https://design-system.service.gov.uk/components/notification-banner).

## Component options

Use options to customise the appearance, content and behaviour of a component when using a macro, for example, changing the text.

See [options table](https://design-system.service.gov.uk/components/notification-banner/#options-notification-banner-example) for details.
3 changes: 3 additions & 0 deletions src/govuk/components/notification-banner/_index.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.govuk-notification-banner {
display: block;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@import "../../base";
@import "./index";
3 changes: 3 additions & 0 deletions src/govuk/components/notification-banner/macro.njk
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{% macro govukNotificationBanner(params) %}
{%- include "./template.njk" -%}
{% endmacro %}
177 changes: 177 additions & 0 deletions src/govuk/components/notification-banner/notification-banner.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
params:
- name: text
type: string
required: true
description: If `html` is set, this is not required. Text to use within the notification banner. If `html` is provided, the `text` argument will be ignored.
Copy link
Contributor

@EoinShaughnessy EoinShaughnessy Oct 21, 2020

Choose a reason for hiding this comment

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

Does "text to use" suggest that users do something with the text once it's inside the notification banner? Suggested tweak: "The text to display within the notification banner."

Also, should this info display at the start of the description, i.e., after Required? This would inform the user what the text option is before they learn about the effect html has on it.

Re first and last sentences — could personalise and phrase in active voice. So, "If you set 'html', this..." and "If you provide 'html', this..."

Copy link
Member Author

@hannalaakso hannalaakso Oct 22, 2020

Choose a reason for hiding this comment

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

We're going to do a separate wider review of the template options docs as this comment touches on most of the components.

- name: html
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the table options display in order of importance? If not, then is there a case for displaying them alphabetically? (I understand, though, if we wouldn't want to change the order because it would make this doc inconsistent with other docs.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a loose order of importance (very loose in fact) and we seem to have settled on a bit of a hierarchy where text/html come first and the classes option is always last. We could definitely review that though.

type: string
required: true
description: If `text` is set, this is not required. HTML to use within the notification banner. If `html` is provided, the `text` argument will be ignored.
Copy link
Contributor

@EoinShaughnessy EoinShaughnessy Oct 21, 2020

Choose a reason for hiding this comment

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

Could personalise first and last sentences and phrase them in the active voice: "If you set 'text', this..." and "If you provide 'html', this..."

Do we say 'parameter' instead of 'argument'?

Should "HTML to use within the notification banner" display at the start of the description, i.e., after Required? This would inform the user what the option is before they learn anything else about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we say 'parameter' instead of 'argument'?

We actually say 'option' 😅 but this might not always be consistent throughout the rest of the tech docs.

- name: title
Copy link
Contributor

Choose a reason for hiding this comment

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

So this option is for the title text in the banner, and the text option (above) is for non-title text in the banner? In the table, should this option display above the text option, to reflect their order in the banner?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. We've tended to have the text/html options first as those could be considered to be the main content. In this particular case, title text is also not required (a placeholder text is used if you don't supply one) whereas text/html are.

type: string
required: false
description: Title text to use within the notification banner. Defaults to 'Important' ('Success' for success type and 'Error' for error type). If `titleHtml` is supplied, the `title` argument will be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Re first sentence — could just say "The title of the notification banner" (if that sounds ok).

What do we mean by “Defaults to ‘Important’” and ‘“‘Success’ for success type and ‘Error’ for error type’”? Also, is “‘Success’ for success type and ‘Error’ for error type’” self-evident (success for success, error for error)?

I think we can delete the parentheses as the style guide says not to use them.

Re last sentence — we could personalise this and make it active, for example, "If you provide 'titleHTML,' the 'title' argument..." (The options above this say "provide" instead of "supply," so I'd use the same wording here.)

Copy link
Member Author

Choose a reason for hiding this comment

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

What do we mean by “Defaults to ‘Important’” and ‘“‘Success’ for success type and ‘Error’ for error type’”?

I agree, this is definitely not clear, will be good to chat about it next week.

- name: titleHtml
type: string
required: false
description: Title HTML to use within the notification banner. If `titleHtml` is provided, the `title` argument will be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we personalise and replace the passive? So: “If you provide 'titleHtml',…”

- name: titleHeadingLevel
type: string
required: false
description: Heading level, from 1 to 6. Default is `2`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark's suggestion: "Sets the heading level of the title. The minimum is 1 and the maximum is 6. The default is 2."

How do we feel about beginning each description with a verb, like in Mark's suggestion? Would it be a good way to communicate the purpose of each option?

- name: type
type: string
required: false
description: If `type` is set to `success` or `error`, the notification banner sets `role` to `alert` and `tabindex` to `-1`, and JavaScript moves the keyboard focus moves to the notification banner when the page loads. If `type` is not set, the notification banner defaults to setting `role` to `region` and using `aria-labelledby` to provide information for users of assistive technologies.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description states what happens when you set type. Do we need to specify what type actually is, though?

Is this option to denote whether the notification banner is purely informational or else an alert box which the screen reader picks up on immediately?

Typo to remove: second "moves" in "Javascript moves the keyboard focus moves to the notification banner..."

Could personalise and make active the second last and last sentences. So, "If you set type to success..." and "If you do not set type..."

I'd also suggest changing "using aria-labelledby" to "uses aria-labelledby." This would be to parallel the active voice used in the previous clause ("...the notification banner defaults...").

- name: role
type: string
required: false
description: Overrides the value of the `role` attribute for the notification banner. Defaults to `region`. If `type` is set to `success` or `error`, defaults to `alert`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description for the type option (above) mentions the role attribute, but doesn’t explain it. Would it help users if this role description displayed before they read about the type option?

“If type is set to success or error, defaults to alert.” It’s the role that defaults (and not the type), is that right? If so, should we explicitly call that out?

Replace passive in last sentence? So: “If you set type to…”

Copy link
Contributor

Choose a reason for hiding this comment

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

The description for the type option (above) mentions the role attribute, but doesn’t explain it. Would it help users if this role description displayed before they read about the type option?

“If type is set to success or error, defaults to alert.” It’s the role that defaults (and not the type), is that right? If so, should we explicitly call that out?

Replace passive in last sentence? So: “If you set type to…”

- name: tabindex
type: string/boolean
required: false
description: Overrides the value of the `tabindex` attribute for the notification banner. Not set by default. If `type` is set to `success` or `error`, defaults to `-1`; the attribute can be removed by setting `tabindex` to `false`;
Copy link
Contributor

@EoinShaughnessy EoinShaughnessy Oct 21, 2020

Choose a reason for hiding this comment

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

The description for the type option above mentions the tabindex attribute, but doesn’t explain it. Would it help users if this tabindex description displayed before they read about the type option?

Are options set by default unless we state otherwise? If so, do we need to call this out somewhere?

Instead of "Not set by default," could we reword the first sentence to "Overrides the value that the browser sets for the tabindex attribute" (or similar)?

“If type is set to success or error, defaults to alert.” Is it the tabindex or the type that defaults?

Suggested tweak for last sentence: “If you set type to success or error, tabindex defaults to -1. To remove the attribute, set tabindex to false.”

- name: titleId
type: string
required: false
description: Overrides the value of the `id` attribute for the title. `id` used by the `aria-labelledby` attribute on the notification banner to provide information to users of assistive technologies. `id` defaults to `govuk-notification-banner-title` if `role` is set to `region`. If `type` is set to `success` or `error`, `id` is not rendered by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could break up the second sentence. So: “...on the notification banner. This provides information…”

Should this display first? “id used by the aria-labelledby attribute on the notification banner.” It defines the option, which might be the first info a user wants to know.

Replace passive? So, “if you set role to region” instead of “if role is set to region” and “If you set type…" instead of "If type is set..."

- name: autoFocus
type: boolean
required: false
description: Moves keyboard focus to the notification banner when the page loads. Defaults to `false`. If `type` is set to `success` or `error`, defaults to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to say "Defaults to false"? Maybe we could emphasise what setting it to true does. So, "If you set initialFocus to true, the keyboard focus moves to the notification banner when the page loads" (or similar).

Could make last sentence active: "If you set type,…"

- name: classes
type: string
required: false
description: Classes to add to the notification banner.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder could we personalise this (e.g. "Classes you want to add to the notification banner").

- name: attributes
type: object
required: false
description: HTML attributes (for example data attributes) to add to the notification banner.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could edit this to remove parentheses (as per style guide advice) and personalise it. So, "HTML attributes that you want to add to the notification banner, for example, data attributes."


examples:
- name: default
data:
text: This publication was withdrawn on 7 March 2014.
- name: with text as html
data:
html: |
<h3 class="govuk-heading-m">This publication was withdrawn on 7 March 2014</h3><p>Archived and replaced by the <a href="#">new planning guidance</a> launched 6 March 2014 on an external website</p>
- name: with type as success
data:
type: success
text: Email sent to example@email.com
- name: with type as error
data:
type: error
text: There was a problem uploading your file. Please try again.
- name: with a list
vanitabarrett marked this conversation as resolved.
Show resolved Hide resolved
data:
html: |
<h3 class="govuk-heading-m">4 files uploaded</h3>
<ul class="govuk-list govuk-list--bullet">
<li><a href="#">government-strategy.pdf</a></li>
<li><a href="#">government-strategy-v2.pdf</a></li>
<li><a href="#">government-strategy-v3-FINAL.pdf</a></li>
<li><a href="#">government-strategy-v4-FINAL-v2.pdf</a></li>
</ul>

# Hidden examples are not shown in the review app, but are used for tests and HTML fixtures

- name: custom title
hidden: true
data:
title: Important information
text: This publication was withdrawn on 7 March 2014.
- name: title as html
hidden: true
data:
titleHtml: <span>Important information</span>
text: This publication was withdrawn on 7 March 2014.
- name: title html as text
hidden: true
data:
title: <span>Important information</span>
text: This publication was withdrawn on 7 March 2014.
- name: custom title heading level
hidden: true
data:
titleHeadingLevel: 3
text: This publication was withdrawn on 7 March 2014.
- name: custom title id
hidden: true
data:
titleId: my-id
text: This publication was withdrawn on 7 March 2014.
- name: custom title id with type as success
hidden: true
data:
type: success
titleId: my-id
text: Email sent to example@email.com
- name: custom title id with type as error
hidden: true
data:
type: error
titleId: my-id
text: There was a problem uploading your file. Please try again.

- name: custom text
hidden: true
data:
text: This publication was withdrawn on 7 March 2014.
- name: html as text
hidden: true
data:
text: <span>This publication was withdrawn on 7 March 2014.</span>

- name: custom role
hidden: true
data:
role: banner
text: This publication was withdrawn on 7 March 2014.
- name: role overridden to region
hidden: true
data:
type: success
role: region
text: Email sent to example@email.com-
- name: custom tabindex
hidden: true
data:
tabindex: 0
text: This publication was withdrawn on 7 March 2014.
- name: tabindex as false and type as success
hidden: true
data:
type: success
tabindex: false
text: Email sent to example@email.com
- name: autoFocus as true
hidden: true
data:
autoFocus: true
text: This publication was withdrawn on 7 March 2014.
- name: autoFocus as false and type as success
hidden: true
data:
type: success
autoFocus: false
text: Email sent to example@email.com

- name: classes
hidden: true
data:
text: This publication was withdrawn on 7 March 2014.
classes: app-my-class
- name: attributes
hidden: true
data:
text: This publication was withdrawn on 7 March 2014.
attributes:
my-attribute: value

- name: with invalid type
hidden: true
data:
type: some-type
text: This publication was withdrawn on 7 March 2014.
65 changes: 65 additions & 0 deletions src/govuk/components/notification-banner/template.njk
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
{%- if params.type == "success" or params.type == "error" %}
{% set successOrError = true %}
{% endif %}

{%- if successOrError %}
{% set typeClass = "govuk-notification-banner--" + params.type %}
{% endif %}

{% if params.role %}
{% set role = params.role %}
{% elif successOrError %}
{# If type is success or error, add `role="alert"` to prioritise the information in the notification banner to users of assistive technologies #}
{% set role = "alert" %}
{% else %}
{# Otherwise add `role="region"` to make the notification banner a landmark to help users of assistive technologies to navigate to the banner #}
{% set role = "region" %}
{% endif %}

{# Check whether to add `data-auto-focus="true"` which focuses the notification banner on page load #}
{% if params.autoFocus is defined %}
{% set autoFocus = params.autoFocus %}
{% elif successOrError %}
{% set autoFocus = true %}
{% endif %}

{% if params.tabindex is defined %}
{% set tabindex = params.tabindex %}
{# Make sure that success or error banner should be focused on page load #}
{% elif successOrError and autoFocus %}
{# Make the notification banner focusable #}
{% set tabindex = "-1" %}
{% endif %}

{%- if params.titleHtml %}
{% set title = params.titleHtml | safe %}
{%- elif params.title %}
{% set title = params.title %}
{%- elif params.type == "success" %}
{% set title = "Success" %}
{%- elif params.type == "error" %}
{% set title = "Error" %}
{%- else %}
{% set title = "Important" %}
{%- endif -%}

<div class="govuk-notification-banner {{ typeClass }} {%- if params.classes %} {{ params.classes }}{% endif -%}" role="{{ role }}"
{%- if tabindex or tabindex === 0 %} tabindex="{{ tabindex }}" {% endif %}
{%- if (role == "region") %} aria-labelledby="{{ params.titleId | default('govuk-notification-banner-title') }}" {%- endif -%}
data-module="govuk-notification-banner"
{%- if autoFocus -%} data-auto-focus="true"{% endif %}
{%- for attribute, value in params.attributes %} {{attribute}}="{{value}}"{% endfor %}>
<div class="govuk-notification-banner__header">
<h{{ params.titleHeadingLevel | default(2) }} class="govuk-notification-banner__title"{% if (role == "region" or params.titleId) %} id="{{ params.titleId | default('govuk-notification-banner-title') }}" {%- endif %}>
{{ title }}
</h{{ params.titleHeadingLevel | default(2) }}>
</div>
<div class="govuk-notification-banner__content">
{%- if params.html -%}
{{ params.html | safe }}
{%- elif params.text -%}
{# Set default style for single line content #}
<p class="govuk-notification-banner__heading">{{ params.text }}</p>
{%- endif -%}
</div>
</div>
Loading