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

Header: Support custom logos #1281

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

wildtangent
Copy link
Contributor

@wildtangent wildtangent commented Jan 23, 2025

This feature adds the ability to override the logo displayed in the Header component.

Different government organisations have different logo requirements including removing the header logo altogether

To pass in a different header logo simply add the logo prop to the Header component and pass in any ReactNode, or pass in null to remove the logo altogether.

Note that the govUK={true} prop will take precedence over the custom logo.

Example usage:

const YourCustomLogo = (props) => (
  <svg 
    // ...
  />
)

return (
  <Header
    logo={<YourCustomLogo />}
  />
)

@wildtangent wildtangent changed the title feat: add logo prop feat: add logo prop to the Header component Jan 24, 2025
@wildtangent wildtangent force-pushed the feature/custom-logo-in-header branch 2 times, most recently from 0d947ff to 0186674 Compare January 24, 2025 10:24
…to be added

chore: add test cases to the Header component to support testing the logo prop

fix: remove unused import
@wildtangent wildtangent force-pushed the feature/custom-logo-in-header branch from 0186674 to 6eb6963 Compare January 24, 2025 10:25
@wildtangent wildtangent marked this pull request as ready for review January 24, 2025 10:25
@@ -94,6 +96,7 @@ export const Header: FC<HeaderProps> = ({
serviceName,
signOutHref,
signOutText = 'Sign out',
logo,
Copy link
Owner

Choose a reason for hiding this comment

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

Would slightly prefer logo: _logo, as that's a style we use elsewhere.

But that's purely stylistic, and I'm happy to fix that later if it doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to align with your coding standards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been updated

@daniel-ac-martin daniel-ac-martin changed the title feat: add logo prop to the Header component Header: Support custom logos Jan 24, 2025
Comment on lines +91 to +92
expect(screen.getByTestId('custom-logo')).toBeInTheDocument()
expect(screen.queryByTestId('crownLogo')).not.toBeInTheDocument()
Copy link
Owner

Choose a reason for hiding this comment

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

I suspect there will be a way to do this with getByRole('img') and maybe getByLabelText() but it's not a big deal and I'm happy to have a dig around later.

wildtangent and others added 4 commits January 31, 2025 13:41
Co-authored-by: Daniel A.C. Martin <daniel-ac-martin@users.noreply.github.com>
Co-authored-by: Daniel A.C. Martin <daniel-ac-martin@users.noreply.github.com>
Co-authored-by: Daniel A.C. Martin <daniel-ac-martin@users.noreply.github.com>
Co-authored-by: Daniel A.C. Martin <daniel-ac-martin@users.noreply.github.com>
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