[WIP] CONSOLE-4062: add custom favicon #14770
Conversation
|
@rhamilto: This pull request references CONSOLE-4062 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
component/core
rhamilto
force-pushed
the
CONSOLE-4062
branch
from
367bdfd to
8ff16de
Compare
rhamilto
force-pushed
the
CONSOLE-4062
branch
from
8ff16de to
28469e9
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rhamilto: This pull request references CONSOLE-4062 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
It is, but we can't use that code here since this is a go html template.
There was a problem hiding this comment.
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.
| .then((newStatusPageData) => setStatusPageData(newStatusPageData)); | ||
| } | ||
| }, [setStatusPageData]); | ||
| // DO NOT COMMIT |
There was a problem hiding this comment.
Temp hack until the new SERVER_FLAGS are available.
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
It is, but we can't use that code here since this is a go html template.
POC demo
Screen.Recording.2025-02-13.at.4.38.04.PM.mov
To test WIP, start
bridgeas follows:./bin/bridge -branding=ocp -statuspage-id="GitHub"cc: @jhadvig