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

[RHOAIENG-17003] Fixing missing RHOAI logo on dark mode. #398

Merged

Conversation

ugiordan
Copy link

Description

JIRA: https://issues.redhat.com/browse/RHOAIENG-17003
fixing the missing RHOAI logo when selecting dark mode on dashboard.

How Has This Been Tested?

Test Impact

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@andrewballantyne
Copy link

Please submit all code changes to the ODH https://github.com/opendatahub-io/odh-dashboard repo.

Thanks!

@andrewballantyne
Copy link

Oops, this is changing the Konflux dockerfile... missed that.

@asanzgom asanzgom self-requested a review December 17, 2024 15:51
@zdtsw
Copy link

zdtsw commented Jan 3, 2025

i wonder:

  1. i do not think rhoai logo is missing if in dark mode, rather it is using the odh-logo.svg
ENV ODH_LOGO=../images/rhoai-logo.svg          ==========> content of the image is also for dark mode, but should it be for light mode instead? 
ENV ODH_LOGO_DARK=../images/rhoai-logo.svg    =============> content of the image is for dark mode

what we should have is a new image which is for the light mode and put in frontend/src/images/ folder
otherwise, regardless light or dark, both end with the same image.
cc @jeff-phillips-18 might have more insights

@thatblindgeye
Copy link

@zdtsw There were a couple of RHOAI logos provided (light theme and dark theme) in the following Jira: https://issues.redhat.com/browse/RHOAIUX-428?focusedId=25985438&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-25985438 (should be a comment from October 31st from Kyle). FWIW A PR I have open actually updates the base rhoai-logo.svg file to the light theme one mentioned in that comment, but I haven't added the dark theme logo (link to the commit in my PR where I made the logo update: opendatahub-io@0876997). Though perhaps updating/adding logos is more appropriate in this PR?

@zdtsw
Copy link

zdtsw commented Jan 7, 2025

@zdtsw There were a couple of RHOAI logos provided (light theme and dark theme) in the following Jira: https://issues.redhat.com/browse/RHOAIUX-428?focusedId=25985438&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-25985438 (should be a comment from October 31st from Kyle). FWIW A PR I have open actually updates the base rhoai-logo.svg file to the light theme one mentioned in that comment, but I haven't added the dark theme logo (link to the commit in my PR where I made the logo update: opendatahub-io@0876997). Though perhaps updating/adding logos is more appropriate in this PR?

thanks for the information!
now i can see https://github.com/red-hat-data-services/odh-dashboard/blob/main/frontend/src/images/rhoai-logo.svg has the light mode content.
we could add the "dark" one in ODH and sync it down here, but still we need update accordingly in this PR ( cannot point to the same file for both light and dark if the intention is to overwrite ODH logo files)
but I leave it to @lucferbux / @andrewballantyne / @christianvogt to make better call :)

@jeff-phillips-18
Copy link

@zdtsw @thatblindgeye IMO, since we already have the rhoai-logo.svg file in the upstream, we ought to have a rhoai-logo-dark.svg in the upstream as well.

@thatblindgeye
Copy link

@ugiordan @zdtsw the PR in ODH got merged that adds the dark theme logo for RHOAi, file name rhoai-logo-dark-theme.svg

Dockerfile.konflux Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Jan 8, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@zdtsw zdtsw requested a review from lucferbux January 8, 2025 08:36
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@d225a40). Learn more about missing BASE report.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #398   +/-   ##
=======================================
  Coverage        ?   85.01%           
=======================================
  Files           ?     1404           
  Lines           ?    32239           
  Branches        ?     9038           
=======================================
  Hits            ?    27408           
  Misses          ?     4831           
  Partials        ?        0           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d225a40...73b69e5. Read the comment docs.

Copy link

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

So far changes look good, I've tested the logos locally and it's working as expected. With latest changes that we've ported to downstream this should be ready to go.

@MohammadiIram MohammadiIram merged commit 467fbda into red-hat-data-services:main Jan 8, 2025
5 of 6 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.

9 participants