Skip to content

fix navbar logo#159

Merged
aojea merged 2 commits into
google:mainfrom
aojea:autopudate
Jun 27, 2026
Merged

fix navbar logo#159
aojea merged 2 commits into
google:mainfrom
aojea:autopudate

Conversation

@aojea

@aojea aojea commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a web app manifest (manifest.json) and links it in the head layout, alongside updating the SVG logo with xlink attributes. The review feedback highlights two key improvements: first, embedding a raster image inside the SVG logo can cause rendering failures due to browser security restrictions, so converting it to native SVG paths or inlining it as a Base64 URI is recommended; second, the hardcoded manifest path should use Hugo's relURL function to prevent broken links when deployed to subdirectories.

Comment thread site/assets/icons/logo.svg Outdated
<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512" width="32" height="32">
<image href="/favicon.png" x="0" y="0" height="512" width="512" />
<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 512 512" width="32" height="32">
<image xlink:href="/favicon.png" href="/favicon.png" x="0" y="0" height="512" width="512" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Embedding an external raster image (like /favicon.png) inside an SVG file using <image> can cause rendering issues. When an SVG is loaded via an <img> tag or CSS background-image, browsers block the loading of external resources (including local relative paths like /favicon.png) for security reasons. This will result in a broken or empty logo in the navbar.

To fix this, you should either:

  1. Convert the logo to native SVG vector paths (e.g., <path>, <circle>), which is the recommended approach for scalability and performance.
  2. Inline the PNG image as a Base64 Data URI.

<!-- SAM Custom Favicons -->
<link rel="icon" type="image/png" href="/favicon.png?v=2">
<link rel="shortcut icon" href="/favicon.ico?v=2">
<link rel="manifest" href="/manifest.json">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Hardcoding the root-relative path /manifest.json can break if the site is ever deployed to a subdirectory (for example, on GitHub Pages or a staging environment).

Using Hugo's relURL function ensures the path is resolved correctly relative to the site's base URL.

Suggested change
<link rel="manifest" href="/manifest.json">
<link rel="manifest" href="{{ 'manifest.json' | relURL }}">

@aojea aojea merged commit 7e91e92 into google:main Jun 27, 2026
9 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.

1 participant