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.

Sign up for GitHub

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

[WIP] CONSOLE-4062: add custom favicon #14770

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] CONSOLE-4062: add custom favicon #14770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42e5017
POC
rhamilto Feb 13, 2025
28469e9
Add event handler
rhamilto Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions frontend/public/components/masthead-toolbar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,15 +607,16 @@ const MastheadToolbarContents = ({ consoleLinks, cv, isMastheadStacked }) => {
);
};

React.useEffect(() => {
if (window.SERVER_FLAGS.statuspageID) {
fetch(`https://${window.SERVER_FLAGS.statuspageID}.statuspage.io/api/v2/summary.json`, {
headers: { Accept: 'application/json' },
})
.then((response) => response.json())
.then((newStatusPageData) => setStatusPageData(newStatusPageData));
}
}, [setStatusPageData]);
// DO NOT COMMIT
Copy link
Member Author

Choose a reason for hiding this comment

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

Temp hack until the new SERVER_FLAGS are available.

// React.useEffect(() => {
// if (window.SERVER_FLAGS.statuspageID) {
// fetch(`https://${window.SERVER_FLAGS.statuspageID}.statuspage.io/api/v2/summary.json`, {
// headers: { Accept: 'application/json' },
// })
// .then((response) => response.json())
// .then((newStatusPageData) => setStatusPageData(newStatusPageData));
// }
// }, [setStatusPageData]);

const setLastConsoleActivityTimestamp = () =>
localStorage.setItem(LAST_CONSOLE_ACTIVITY_TIMESTAMP_LOCAL_STORAGE_KEY, Date.now().toString());
Expand Down
2 changes: 1 addition & 1 deletion frontend/public/components/masthead.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export const Masthead = React.memo(({ isMastheadStacked, isNavOpen, onNavToggle
<MastheadBrand>
<MastheadLogo
component="a"
aria-label={window.SERVER_FLAGS.customLogoURL ? undefined : details.productName}
aria-label={details.productName}
data-test="masthead-logo"
{...logoProps}
>
Expand Down
Binary file added frontend/public/imgs/github-dark.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added frontend/public/imgs/github.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
29 changes: 25 additions & 4 deletions frontend/public/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@
[[ end ]] [[ if eq .ServerFlags.Branding "rosa" ]]
<title>Red Hat OpenShift Service on AWS</title>
<meta name="application-name" content="Red Hat OpenShift Service on AWS" />
[[ end ]] [[ if eq .ServerFlags.Branding "okd" ]]
<link rel="shortcut icon" href="<%= require('./imgs/okd-favicon.png') %>" />
[[ end ]]
<!-- [[/* if ne .ServerFlags.CustomFavicon */]] -->
[[ if ne .ServerFlags.StatuspageID "GitHub" ]] [[ if eq .ServerFlags.Branding "okd" ]]
<link rel="icon" href="<%= require('./imgs/okd-favicon.png') %>" />
<link
rel="apple-touch-icon-precomposed"
sizes="144x144"
Expand All @@ -42,7 +44,7 @@
content="<%= require('./imgs/okd-mstile-144x144.png') %>"
/>
[[ else ]]
<link rel="shortcut icon" href="<%= require('./imgs/openshift-favicon.png') %>" />
<link rel="icon" href="<%= require('./imgs/openshift-favicon.png') %>" />
<link
rel="apple-touch-icon-precomposed"
sizes="144x144"
Expand All @@ -55,17 +57,36 @@
content="<%= require('./imgs/openshift-mstile-144x144.png') %>"
/>
[[ end ]] [[ end ]]
<!-- [[/* end */]] -->
[[ end ]]
<!-- update this condition to use the custom favicon serverflag -->
[[ if eq .ServerFlags.StatuspageID "GitHub"]]
<link rel="icon" href="<%= require('./imgs/github.png') %>" id="custom-favicon" />
[[ end ]]

<meta name="description" content="" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<script type="text/javascript" nonce="[[ .ScriptNonce ]]">
const darkModeMediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
window.SERVER_FLAGS = [[ .ServerFlags ]];
let theme = localStorage.getItem('bridge/theme') || 'systemDefault';
if (theme === 'systemDefault' && window.matchMedia('(prefers-color-scheme: dark)').matches) {
// update this condition to use the custom favicon serverflag
Copy link
Contributor

Choose a reason for hiding this comment

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

So far in my PR I'm not setting flags for favicon nor masthead as their presence is known only after the given configuration got parsed ( the new CLI argument/yaml field customLogoFiles is used both masthead and favicons, unlike e.g. statuspage-id and the user may only specify masthead). Only related information I currently provide console is the .ServerFlags.CustomLogoURL containing the custom-logo endpoint, which is not enough to determine favicon presence.

Settings favicon/masthead flag would require additional logic in bridge, but the resulting flag check on frontend would be somewhat simpler than confirming the existence of favicon through the API endpoint. WDYT @jhadvig

Copy link
Member Author

Choose a reason for hiding this comment

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

My assumption is that we'll add a flag or flags so that we can make use of them in index.html so we're consistent about where we're setting the favicon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! previously thought there is some neat way of changing the favicon from the react app, but having the control within the index.html seems more intuitive because the favicon logic is right where it’s applied.

The flags seems like the way to go 👍 will take a look and try to come up how/when to set those.

[[ if eq .ServerFlags.StatuspageID "GitHub"]]
let link = document.getElementById("custom-favicon");
let faviconDark = '<%= require("./imgs/github-dark.png") %>';
darkModeMediaQuery.addEventListener('change', (e) => {
link.href = e.matches ? faviconDark : '<%= require("./imgs/github.png") %>';
});
[[ end ]]
if (theme === 'systemDefault' && darkModeMediaQuery.matches) {
theme = 'dark';
}
if (theme === 'dark') {
document.documentElement.classList.add('pf-v6-theme-dark', 'pf-v5-theme-dark'); // legacy pf-theme-dark class needed for PF5 component styling
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the assignment of the 'pf-v6-theme-dark' / 'pf-v5-theme-dark' needed in the script tag of the html? Think the same logic is being handled by ThemeProvider

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, but we can't use that code here since this is a go html template.

Copy link
Contributor

@Mylanos Mylanos Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, what I was just trying to point out is if this is not a repeated logic. Just asking since removing this part of code hasn't really affected the theming behavior, in my local testing testing atleast. I don't have strong opinions on this, so we can just as well keep it.

// update this condition to use the custom favicon serverflag
[[ if eq .ServerFlags.StatuspageID "GitHub"]]
link.href = faviconDark;
[[ end ]]
}
</script>
</head>
Expand Down