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 nunjucks param descriptions for the notification banner component #2001

Conversation

EoinShaughnessy
Copy link
Contributor

@EoinShaughnessy EoinShaughnessy commented Oct 30, 2020

I replaced some instances of the passive voice with the active voice, and inserted the word "you" at various points. This was to make the content more immediate and personalise it a tad for users.

I also reordered some sentences. This was to frontload info about the purpose of the options.

Please holler at me re any follow-up questions! :)

Fixes #1997

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2001 October 30, 2020 11:58 Inactive
@36degrees 36degrees changed the base branch from master to feature/notification-banner October 30, 2020 12:53
@hannalaakso
Copy link
Member

Thanks @EoinShaughnessy 👍

@vanitabarrett or @36degrees I wonder if you might be able to review this one as I worked on the content changes with Eion.

@hannalaakso hannalaakso removed their request for review November 4, 2020 11:20
@36degrees 36degrees changed the title Notification banner eoin nunjucks edit Update nunjucks param descriptions for the notification banner component Nov 4, 2020
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2001 November 13, 2020 08:56 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2001 November 13, 2020 08:56 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2001 November 13, 2020 08:58 Inactive
@EoinShaughnessy
Copy link
Contributor Author

@vanitabarrett Hi! I've added your edits into the content. Do I now need to tidy the trail of commits somehow?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2001 November 13, 2020 09:59 Inactive
@EoinShaughnessy EoinShaughnessy force-pushed the notification-banner-eoin-nunjucks-edit branch from 676032c to 093bc70 Compare November 16, 2020 09:58
vanitabarrett pushed a commit that referenced this pull request Nov 18, 2020
NOTE: this PR does not update the macro documentation - this will be done separately in #2001

We have decided to keep the aria-labelledby attribute on alert banners too, to make things consistent with the error summary. We were seeing some odd behaviour when testing with NVDA and Voiceover, but this change did not seem to have a negative impact and it also means that when we next revisit the component we're at least working from a place where everything is consistent to start with.
@36degrees
Copy link
Member

Superseded by #2024

@36degrees 36degrees closed this Nov 19, 2020
@36degrees 36degrees deleted the notification-banner-eoin-nunjucks-edit branch November 19, 2020 16:25
vanitabarrett pushed a commit that referenced this pull request Nov 23, 2020
NOTE: this PR does not update the macro documentation - this will be done separately in #2001

We have decided to keep the aria-labelledby attribute on alert banners too, to make things consistent with the error summary. We were seeing some odd behaviour when testing with NVDA and Voiceover, but this change did not seem to have a negative impact and it also means that when we next revisit the component we're at least working from a place where everything is consistent to start with.
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.

None yet

5 participants