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

Jump to bottom

fix(favicon): re-embed dynamic favicon into index.html #2408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(favicon): re-embed dynamic favicon into index.html #2408

wants to merge 1 commit into from
+20 −5

Conversation

logonoff
Copy link

@logonoff logonoff commented Feb 18, 2025 •
edited
Loading

Which issue(s) does this PR fix

fixes https://issues.redhat.com/browse/RHIDP-6015

Description

  • do not show janus favicons if a custom favicon is set, so that there are no issues with which favicon should take priority
  • do not use the "Janus Community Showcase" metadata description if app.title is set
  • move the favicon ConfigUpdaterto outside the AppRouter component so that it can work in the OauthRequestDialog

before:
before

after:
after

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

the following shouldn't happen:

  • the Janus favicon flashes momentarily before the custom one is set
  • the Janus favicon apperas in the login screen
  • the description metadata in the HTML is "Janus Community Showcase" if app.title is set

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 18, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign nilgaar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions GitHub Actions
Copy link
Contributor

The image is available at:

@logonoff logonoff force-pushed the RHIDP-6015 branch 3 times, most recently from 8f595d4 to 88115a5 Compare February 18, 2025 19:27
@logonoff logonoff changed the title fix(RHIDP-6015): re-embed dynamic favicon into index.html fix(favicon): re-embed dynamic favicon into index.html Feb 18, 2025
@github-actions GitHub Actions
Copy link
Contributor

The image is available at:

christoph-jerolimov
christoph-jerolimov requested changes Feb 18, 2025
View reviewed changes
Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

I must test this, but I think you can change the default here from Janus to RHDH:

packages/app/src/components/AppBase/AppBase.tsx
@@ -75,8 +75,8 @@ const AppBase = () => {
<AppProvider>
<AlertDisplay />
<OAuthRequestDialog />
<ConfigUpdater />
Copy link
Member

Choose a reason for hiding this comment

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

Does this also help that the favicon isn't loaded on each page change? :)

Copy link
Author

Choose a reason for hiding this comment

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

This helps with the favicon not loading in the login page, not sure if it helps the favicon not loading with each page change

@logonoff
fix(favicon): re-embed dynamic favicon into index.html
877d4e2 
fixes https://issues.redhat.com/browse/RHIDP-6015

- do not show janus favicons if a custom favicon is set, so that there are no issues with which favicon should take priority
- do not use the "Janus Community Showcase" metadata description if `app.title` is set
- move the favicon `ConfigUpdater`to outside the `AppRouter` component so that it can work in the `OauthRequestDialog`

before:
![before](https://i.imgur.com/XC50VxV.png)

after:
![after](https://i.imgur.com/konWQrQ.png)
@github-actions GitHub Actions
Copy link
Contributor

The image is available at:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzemanov dzemanov Awaiting requested review from dzemanov

@its-mitesh-kumar its-mitesh-kumar Awaiting requested review from its-mitesh-kumar

@christoph-jerolimov christoph-jerolimov Awaiting requested review from christoph-jerolimov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@logonoff @christoph-jerolimov