Skip to content

Add notification banner component#284

Merged
colinrotherham merged 13 commits intoNHSDigital:rollup-directivesfrom
robkerrybjss:add-notification-banner-component
Oct 13, 2025
Merged

Add notification banner component#284
colinrotherham merged 13 commits intoNHSDigital:rollup-directivesfrom
robkerrybjss:add-notification-banner-component

Conversation

@robkerrybjss
Copy link
Copy Markdown
Contributor

No description provided.

@robkerrybjss
Copy link
Copy Markdown
Contributor Author

Fixes #283

Copy link
Copy Markdown
Collaborator

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Love seeing this PR opened!

I've added a few comments but nothing huge. We need to carefully review the design system guidance and add a few missing things.

Verifying all the NHS.UK frontend examples work would be really useful too

Think it's only the task list component missing after this 😎

Comment thread src/components/content-presentation/notification-banner/NotificationBanner.tsx Outdated
Comment thread docs/upgrade-to-6.0.md Outdated
@robkerrybjss
Copy link
Copy Markdown
Contributor Author

Thanks @colinrotherham, I've learned a lot by doing this! Really appreciate you taking the time to give this a thorough review.

@robkerrybjss
Copy link
Copy Markdown
Contributor Author

Forgot to say on Friday, all the changes you've asked for are in now!

@colinrotherham
Copy link
Copy Markdown
Collaborator

Thanks @robkerrybjss, I'm going to have a look at this now 🙌

I've had to adjust childIsOfComponentType() slightly for React Server Components (RSC) in #285 (see commit) but I'll check everything still works here in your PR too

Comment thread docs/upgrade-to-6.0.md

```html
<main class="nhsuk-main-wrapper id="maincontent">
<main class="nhsuk-main-wrapper" id="maincontent">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for spotting this!

Copy link
Copy Markdown
Collaborator

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Excellent work here!

I'm just going to rebase onto #285 so we can get it into the next beta

I'm going to fix a few problems I've caused, bear with

Comment thread stories/Content Presentation/NotificationBanner.stories.tsx Outdated
Comment thread stories/Content Presentation/NotificationBanner.stories.tsx Outdated
Comment thread stories/Content Presentation/NotificationBanner.stories.tsx Outdated
Comment thread stories/Content Presentation/NotificationBanner.stories.tsx Outdated
Comment thread src/components/content-presentation/notification-banner/NotificationBanner.tsx Outdated
@colinrotherham colinrotherham changed the base branch from main to rollup-directives October 13, 2025 14:08
@colinrotherham colinrotherham force-pushed the add-notification-banner-component branch from 46149e3 to 1c43ecd Compare October 13, 2025 14:13
Comment thread src/components/content-presentation/notification-banner/NotificationBanner.tsx Outdated
Comment thread src/components/content-presentation/notification-banner/NotificationBanner.tsx Outdated
Comment thread src/components/content-presentation/notification-banner/NotificationBanner.tsx Outdated
@colinrotherham colinrotherham self-requested a review October 13, 2025 16:07
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Thanks @robkerrybjss

Will merge this into #285 and get another beta released

@colinrotherham colinrotherham merged commit 37641b4 into NHSDigital:rollup-directives Oct 13, 2025
3 checks passed
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.

2 participants