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

Ensure logo-nhs.svg is a valid SVG file. #657

Merged
merged 5 commits into from
Nov 18, 2020

Conversation

codebymikey
Copy link
Contributor

@codebymikey codebymikey commented Nov 12, 2020

Add the xlink namespace to address the 'Unbound namespace prefix: "xlink"' error.

Description

The SVG is currently not valid.

Checklist

Add the xlink namespace to address the 'Unbound namespace prefix: "xlink"' error.
@chrimesdev
Copy link
Member

chrimesdev commented Nov 12, 2020

Hey @codebymikey thanks for opening this pull request.

Do you have a bit more information about the error you were getting so I can understand the context of this change? Where are you getting the 'Unbound namespace prefix: "xlink"' error?

We are aware of some issues affecting HTML validation with the fallback png logo (#572) but this doesn't seem to be related.

@chrimesdev chrimesdev self-requested a review November 12, 2020 18:14
@codebymikey
Copy link
Contributor Author

I just noticed that the file wasn't valid in general (none of the svg viewers I found online could open it), see Github's own SVG preview breaking:
https://github.com/nhsuk/nhsuk-frontend/blob/e3850ef/packages/assets/logos/logo-nhs.svg
vs
https://github.com/nhsuk/nhsuk-frontend/blob/28c9253/packages/assets/logos/logo-nhs.svg

Viewing the svg directly in Chrome spits out an error message regarding the namespace prefix, but still manages to render.

The error I referenced in particular was coming from running the assets through gulp-image's svgo optimizer which apparently uses sax-js under the hood:

https://github.com/isaacs/sax-js/blob/5aee2163d55cff24b817bbf550bac44841f9df45/lib/sax.js#L809-L813

@chrimesdev
Copy link
Member

Thanks @codebymikey that makes sense - I'm surprised we haven't spotted this before!

The change looks good but there are a couple of other places that we'll need to make this change;

Header component template file (Line 17) https://github.com/nhsuk/nhsuk-frontend/blob/master/packages/components/header/template.njk#L17

Header component template file (Line 28) https://github.com/nhsuk/nhsuk-frontend/blob/master/packages/components/header/template.njk#L28

Header component README file (There are a couple of places here) https://github.com/nhsuk/nhsuk-frontend/blob/master/packages/components/header/README.md

We'll need a CHANGELOG entry too under 4.0.1 as a fix - though we can pick this up if you prefer.

Thanks again.

@chrimesdev chrimesdev added the 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) label Nov 13, 2020
@codebymikey
Copy link
Contributor Author

Applied the namespace to all known instances of xlink in the codebase.

I'll leave the CHANGELOG part to you guys to handle.

Copy link
Member

@chrimesdev chrimesdev left a comment

Choose a reason for hiding this comment

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

Thanks @codebymikey looks good, I'll update the CHANGELOG before the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants