Refactor logo usage

[AMoo-Miki]

Description

Refactor logo usage
Also:

• Move logos to a central location
• Make the loading spinner color-scheme-aware
• Recreate OverviewPageHeader, HomeIcon, HeaderLogo, SolutionTitle, Welcome, Overview tests
• Enhance ExitFullScreenButton, Header tests
• Tinified favicon assets
• Introduce a custom object snapshot serializer for testing

Details

This change is merely to refactor logo, mark, and loading spinner imagery in one place.

• Many components had logic of their own for parsing the branding and choosing imagery; this change pulls that logic to a central logos. This change also removes all of the individual unit tests that dealt with the image selection.
• The logos need to be made available to both server, for rendering the <Template />, and public, for use across the frontend; hence it is created in common.
• The logos are exposed to the frontend via the chrome service but depend on the branding metadata that are compiled by the rendering service.
• The next step is to move the parsing, validation, and serving of custom branding into a service of their own and use schema and its validation capabilities.
  • In this change, colorScheme is attached to logos and branding.darkMode is still used in some places; the branding service will take that over and expose it directly on chrome.
  • branding.applicationTitle continues to be messed up and will be taken over by the new branding service.
  • favicon in general is a messed up across browsers; OSD's use of favicons is also messed up. Branding's favicon is also very limiting as no user will be able to customize them with only an SVG or only a PNG. While logos will own the delivery of the favicon in the upcoming change, we will have to deal with the favicons differently later.

To reviewers

While I have tried to make the tests do real testing, as opposed to just taking snapshots, there is a lot hidden in the snapshots; please start your review by looking at getLogos and then validate that snapshots are doing what the tests expect them to be doing.

Issues Resolved

Partially fixes #895

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[codecov]

Codecov Report

Merging #4702 (d25248f) into main (a9a8122) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 96.20%.

@@           Coverage Diff           @@
##             main    #4702   +/-   ##
=======================================
  Coverage   66.29%   66.30%           
=======================================
  Files        3320     3322    +2     
  Lines       63959    63919   -40     
  Branches    10136    10115   -21     
=======================================
- Hits        42402    42381   -21     
+ Misses      19068    19056   -12     
+ Partials     2489     2482    -7     

Flag	Coverage Δ
Linux_1	34.84% <57.35%> (+0.08%)	⬆️
Linux_2	55.13% <95.52%> (+0.03%)	⬆️
Linux_3	43.40% <56.33%> (+0.12%)	⬆️
Linux_4	35.10% <63.29%> (+0.05%)	⬆️
Windows_1	34.85% <57.35%> (+0.08%)	⬆️
Windows_2	55.10% <95.52%> (+0.03%)	⬆️
Windows_3	43.40% <56.33%> (+0.12%)	⬆️
Windows_4	35.10% <63.29%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/core/public/chrome/chrome_service.mock.ts	100.00% <ø> (ø)
src/core/public/chrome/ui/header/home_loader.tsx	17.14% <ø> (ø)
src/core/server/rendering/rendering_service.tsx	66.66% <ø> (ø)
src/core/server/rendering/views/styles.tsx	100.00% <ø> (ø)
...ication/embeddable/dashboard_container_factory.tsx	0.00% <ø> (ø)
...ication/embeddable/viewport/dashboard_viewport.tsx	85.18% <ø> (ø)
src/plugins/dashboard/public/plugin.tsx	0.00% <ø> (ø)
...plugins/home/public/application/components/home.js	91.48% <ø> (ø)
src/core/common/logos/get_logos.ts	93.02% <93.02%> (ø)
src/core/common/logos/get_logos.mock.ts	100.00% <100.00%> (ø)
... and 14 more

... and 5 files with indirect coverage changes

[AMoo-Miki]

This made the favicon show pink on Safari.

[BSFishy]

Are we able to just remove it and let the SVG provide the color?

[AMoo-Miki]

We probably should; the whole favicons part needs revamping.

[ashwin-pc]

Nice job centralizing the logo logic but you can simplify this function and make it a lot more manageable too. Something like this:

export const getLogos = (branding = {}, serverBasePath) => {
  const {
    logo: { defaultUrl: customLogoUrl, darkModeUrl: customDarkLogoUrl } = {},
    mark: { defaultUrl: customMarkUrl, darkModeUrl: customDarkMarkUrl } = {},
    loadingLogo: { defaultUrl: customSpinnerUrl, darkModeUrl: customDarkSpinnerUrl } = {},
    darkMode = false,
  } = branding;

  const getUrl = (dark, customDarkUrl, customUrl, defaultDarkUrl, defaultLightUrl) => 
    (dark && customDarkUrl) || customUrl || defaultDarkUrl || defaultLightUrl;

  const getType = (customUrl, defaultUrl) => customUrl ? 'custom' : defaultUrl ? 'default' : '';

  const getLogo = (name, customUrl, defaultLightUrl, defaultDarkUrl) => ({
    url: getUrl(darkMode, customUrl, customLogoUrl, defaultDarkUrl, defaultLightUrl),
    type: getType(customUrl, defaultLightUrl),
    light: { url: customUrl || defaultLightUrl },
    dark: { url: customDarkUrl || customUrl || defaultDarkUrl },
  });

  const logos = {
    OpenSearch: getLogo('OpenSearch', customLogoUrl, defaultOnLightOpenSearch, defaultOnDarkOpenSearch),
    OpenSearchDashboards: getLogo('OpenSearchDashboards', customLogoUrl, defaultOnLightOpenSearchDashboards, defaultOnDarkOpenSearchDashboards),
    Mark: getLogo('Mark', customMarkUrl, defaultOnLightMark, defaultOnDarkMark),
    CenterMark: getLogo('CenterMark', customMarkUrl, defaultOnLightCenterMark, defaultOnDarkCenterMark),
    Spinner: getLogo('Spinner', customSpinnerUrl, customMarkUrl || defaultOnLightSpinner, 
                     customDarkSpinnerUrl || customSpinnerUrl || customDarkMarkUrl || defaultOnDarkSpinner),
    colorScheme: darkMode ? 'dark' : 'light',
  };

  return deepFreeze(logos);
};

[AMoo-Miki]

While it doesn't matter in the grand scheme of things, the more verbose code that runs in the browser uses lesser memory and runs quicker. In other words, I intentionally intentionally dumbed down the logic but I don't have any strong feelings. Do you still want it made complicated?

[joshuarrrr]

Personally, I worry more about the extensive use of mutable variables - they make i much more likely that a subsequent change to this file has some unintended side effect if the developer doesn't pay close attention to the order of assignment and conditionals (and variable names which can be tricky to keep straight). I think there's probably a happy medium that prefers const and conditionals encapsulated in functions but still is more explicit in the actual fallback paths.

[AMoo-Miki]

I took both your inputs; i will rework this.

[AMoo-Miki]

Removed the verbosity from it; I tried to keep it as uncomplicated as I could.

[joshuarrrr]

Nice, I think this is the best version yet - nice balance of readability and maintainability.

[BSFishy]

LGTM, there are still a few typedefs that could be consolidated but that's not a blocker

[joshuarrrr]

@AMoo-Miki I'm still in-progress reviewing everything in plugins, but I had enough comments and questions so far, I thought I should go ahead and share. Overall, looks like a great improvement!

[joshuarrrr]

Personally, I worry more about the extensive use of mutable variables - they make i much more likely that a subsequent change to this file has some unintended side effect if the developer doesn't pay close attention to the order of assignment and conditionals (and variable names which can be tricky to keep straight). I think there's probably a happy medium that prefers const and conditionals encapsulated in functions but still is more explicit in the actual fallback paths.

[joshuarrrr]

overall, looks good, although I'll admit that my concentration was wandering a bit by the end. I actually think some of this logo/mark usage is kind of weird from a design perspective (such as the overview page where it seems duplicative), or the exit fullscreen button, where it seems gratuitous, but it's at least nice to have all these usages unified.

[joshuarrrr]

When we specify these paths directly, doesn't it result in dark/light unaware logos?

[AMoo-Miki]

yes but i dont see them used at all.

[joshuarrrr]

🤦 I

[AMoo-Miki]

LGTM, there are still a few typedefs that could be consolidated but that's not a blocker

I thought i had done it; will do now.

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-4702-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d7c5577811a3ba7bcc0bb28b188d77a8e95a8237
# Push it to GitHub
git push --set-upstream origin backport/backport-4702-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4702-to-2.x.