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

CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 #1753

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

Conversation

cajieh
Copy link

@cajieh cajieh commented Feb 11, 2025

No description provided.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 11, 2025

@cajieh: This pull request references CONSOLE-4448 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 openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 11, 2025
@openshift-ci openshift-ci bot requested review from deads2k and joepvd February 11, 2025 22:03
Copy link
Contributor

openshift-ci bot commented Feb 11, 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 jerpeter1 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

@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch 2 times, most recently from 05edd67 to 3cd02ff Compare February 12, 2025 20:04
Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

Thank you @cajieh for the PR, adding couple of comments.
Lets sync on the console-config API and the expected console-operator behaviour.

### Attributes Description

#### customLogos
- `type`: Specifies the type of logo. Possible values are `Masthead` and `Favicon`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `type`: Specifies the type of logo. Possible values are `Masthead` and `Favicon`.
- `type`: Enumerate which specifies the type of custom logo. Available custom logo types are `Masthead` and `Favicon`.

- `themes`: A list of themes for which the custom logo is defined.

#### themes
- `type`: Specifies the theme type. Possible values are `Light` and `Dark`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `type`: Specifies the theme type. Possible values are `Light` and `Dark`.
- `type`: Enumerate which specifies the type of theme. Available theme types are `Light` and `Dark`.

#### file
- `key`: The key or path to the logo file.
- `name`: The name identifier for the custom logo.

Copy link
Author

@cajieh cajieh Feb 14, 2025

Choose a reason for hiding this comment

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

I think it would be also good a proposal for the console-config API field, to align on the structure.

I think I don't understand this comment, please clarify?

Copy link
Member

Choose a reason for hiding this comment

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

lets just leave that one from the proposal... sorry for confusion


### Risks and Mitigations

#### The OpenShift Console is optional and could be disabled
Copy link
Member

Choose a reason for hiding this comment

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

This section could be removed.

│ ├── name
└── ...

### API Extensions
Copy link
Member

Choose a reason for hiding this comment

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

This section could be removed.




### Version Skew Strategy
Copy link
Member

Choose a reason for hiding this comment

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

This section could be removed.

### Version Skew Strategy


### Operational Aspects of API Extensions
Copy link
Member

Choose a reason for hiding this comment

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

This section could be removed.


N/A

#### Support Procedures
Copy link
Member

Choose a reason for hiding this comment

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

This section could be removed.


N/A

## Implementation History
Copy link
Member

Choose a reason for hiding this comment

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

This section could be removed.


N/A

### Upgrade / Downgrade Strategy
Copy link
Member

Choose a reason for hiding this comment

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

This section could be removed.

@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch 2 times, most recently from 9987f2b to 3ab0cef Compare February 13, 2025 19:39
@cajieh cajieh changed the title [WIP]: CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 Feb 13, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2025
@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch from 3ab0cef to d46bed6 Compare February 13, 2025 19:48

1. Users could set both the `CustomLogoFile` and `CustomLogos` APIs. When both APIs are set.

To mitigate this challenge, the `CustomLogos` API configuration will take precedence over the old `CustomLogoFile` field.
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to prevent both values from being set through validation? @everettraven ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use CEL validation on the parent type to prevent both those fields from being set.


#### Removing a deprecated feature

The current custom logo feature is deprecated and will be removed at some point in the future. Users are encouraged to transition to the new custom logo configuration that supports light and dark theme modes for the masthead and favicon.
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me what API tier our operator config is, but I wouldn't expect us to remove the deprecated field anytime soon. It's not a huge burden to support both. If it's tier 1, we aren't able to remove it until the next major version.

https://docs.openshift.com/container-platform/4.17/rest_api/overview/understanding-api-support-tiers.html

#### file
- `key`: The key or path to the logo file.
- `name`: The name identifier for the custom logo.

Copy link
Author

@cajieh cajieh Feb 14, 2025

Choose a reason for hiding this comment

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

I think it would be also good a proposal for the console-config API field, to align on the structure.

I think I don't understand this comment, please clarify?

@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch from d46bed6 to d2c4c2a Compare February 14, 2025 18:54
@cajieh
Copy link
Author

cajieh commented Feb 16, 2025

/test markdownlint

@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch 4 times, most recently from 5d583f4 to df01baf Compare February 20, 2025 14:03

#### Removing a deprecated feature

The current custom logo feature is deprecated and will be removed at some point in the future. Users are encouraged to transition to the new custom logos configuration that supports light and dark theme modes for the masthead and favicon. The new custom logos feature also includes support for a default theme for all unspecified themes.
Copy link
Member

Choose a reason for hiding this comment

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

Lets also mention the name of the deprecated CustomLogo field and its path customization.customLogo


1. Users could set both the `CustomLogoFile` and `CustomLogos` APIs. The `CustomLogos` API configuration will take precedence over the old `CustomLogoFile` field.

2. If only the dark or light theme is specified, the default OCP logo will be used for the unspecified theme.
Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase this to:
Each of the Console supported themes can be configured individually by setting either the Light or Dark theme type or by applying a default theme to all supported themes using the Default theme type. If the Default theme type is set along with a specific Dark or Light theme, the specific theme setting will override the default one.

#### file
- `key`: The key or path to the logo file.
- `name`: The name identifier for the custom logo.

Copy link
Member

Choose a reason for hiding this comment

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

lets just leave that one from the proposal... sorry for confusion

- `key`: The key or path to the custom logo file.
- `name`: The name of the ConfigMap containing the custom logo file.

### Test Plan
Copy link
Member

Choose a reason for hiding this comment

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

Lets list tests which we will deliver:

  • unit tests in the API repo
  • unit and e2e tests in the console-operator repo
  • e2e tests in the console repo


### Test Plan

### Graduation Criteria
Copy link
Member

Choose a reason for hiding this comment

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

this will be needed only if we will be forced to put the API behind feature gate. And since technically this is API change is addressing a bug, lets remove it 🤞

Copy link
Member

Choose a reason for hiding this comment

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

Please also remove the unused sections below

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Screenshot 2025-02-20 at 2 21 21 PM


## Summary

The OpenShift Container Platform (OCP) web console is currently being upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). Consequently, a single custom logo may not be suitable for both theme modes.
Copy link
Member

Choose a reason for hiding this comment

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

Lets mention the upgrade in the past tense.

Suggested change
The OpenShift Container Platform (OCP) web console is currently being upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). Consequently, a single custom logo may not be suitable for both theme modes.
The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). As a result, a single custom logo may not be suitable for both theme modes.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I thought of this but was wondering if the actual upgrade happens after the release.


## Background info

The OpenShift Container Platform (OCP) web console is currently being upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). Consequently, a single custom logo may not be suitable for both modes.
Copy link
Member

Choose a reason for hiding this comment

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

same as Summary section


## Motivation

The OpenShift Container Platform (OCP) web console is currently being upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). Consequently, a single custom logo may not be suitable for both theme modes. This is evident with the existing OKD and OpenShift logos, which assume a dark masthead background and include white text.
Copy link
Member

Choose a reason for hiding this comment

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

same as above...

I would suggest to:

Summary
The OpenShift Container Platform (OCP) web console is being upgraded to PatternFly 6, which introduces a masthead color that changes based on the selected theme mode (light or dark). As a result, a single custom logo may not be suitable for both modes, potentially affecting visibility and aesthetics.

Motivation
The existing OKD and OpenShift logos are designed for a dark masthead background and include white text, making them less effective in a light theme. To ensure logos remain visible and visually appealing in both light and dark themes, users need the ability to add custom logos for the masthead and favicon that adapt to the theme mode. To support this, a new API endpoint will be introduced, allowing users to specify different logos for light and dark themes.


2. If only the dark or light theme is specified, the default OCP logo will be used for the unspecified theme.

3. Handling different custom logos for light and dark themes may increase the complexity of the backend and UI logic. Tests are added to validate the new logic and a fallback is implemented to default to the OCP logo if any theme is missing.
Copy link
Member

Choose a reason for hiding this comment

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

lets remove this this item since part of it will be mentioned above and the testing part is part of the Test Plan section


3. Handling different custom logos for light and dark themes may increase the complexity of the backend and UI logic. Tests are added to validate the new logic and a fallback is implemented to default to the OCP logo if any theme is missing.

4. Users might experience confusion with the introduction of new logo configuration options, especially if they are familiar with the old method, which may soon be deprecated. Provide a comprehensive documentation and tools that guide users through the transition. Include clear instructions about the changes and their benefits.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
4. Users might experience confusion with the introduction of new logo configuration options, especially if they are familiar with the old method, which may soon be deprecated. Provide a comprehensive documentation and tools that guide users through the transition. Include clear instructions about the changes and their benefits.
4. Users might experience confusion with the introduction of new logo configuration options, especially if they are familiar with using previous method, represented by the `CustomLogo` field, which will be deprecated. Provide a comprehensive documentation that guide users through the transition. Include clear instructions about the changes and their benefits.

@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch from df01baf to d210f79 Compare February 20, 2025 19:54
@cajieh
Copy link
Author

cajieh commented Feb 20, 2025

Docs Approver
/assign @opayne1

Copy link

@opayne1 opayne1 left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple docs tweaks/suggestions. Let me know if you have questions. Thanks!


## Summary

The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). As a result, a single custom logo may not be suitable for both theme modes.
Copy link

Choose a reason for hiding this comment

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

Suggested change
The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). As a result, a single custom logo may not be suitable for both theme modes.
The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on light or dark mode. As a result, a single custom logo may not be suitable for both themes.

Can we just say either "both themes" or "both modes"?


The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). As a result, a single custom logo may not be suitable for both theme modes.

Add the ability to specify custom logos to support light and dark theme modes for the masthead and favicon.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Add the ability to specify custom logos to support light and dark theme modes for the masthead and favicon.
Patternfly 6 adds the ability to specify custom logos to support light and dark theme modes for the masthead and favicon.

I am not sure this is correct here but this sentence needs a subject.


## Background info

The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). As a result, a single custom logo may not be suitable for both theme modes.
Copy link

Choose a reason for hiding this comment

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

Suggested change
The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). As a result, a single custom logo may not be suitable for both theme modes.
The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on light or dark mode. As a result, a single custom logo may not be suitable for both theme modes.


The OpenShift Container Platform (OCP) web console was upgraded to PatternFly 6. In PatternFly 6, the masthead color changes based on the mode (light or dark). As a result, a single custom logo may not be suitable for both theme modes.

To address this, we need to allow users to add custom logos for both the masthead and favicon that are compatible with light and dark themes. This ensures that the logos are always visible and aesthetically pleasing, regardless of the theme mode.
Copy link

Choose a reason for hiding this comment

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

Suggested change
To address this, we need to allow users to add custom logos for both the masthead and favicon that are compatible with light and dark themes. This ensures that the logos are always visible and aesthetically pleasing, regardless of the theme mode.
To address this, we need to allow users to add custom logos compatible with light and dark themes for both the masthead and favicon. This ensures that the logos are always visible and aesthetically pleasing, regardless of the theme mode.


## Motivation

The existing OKD and OpenShift logos are designed for a dark masthead background and include white text, making them less effective in a light theme. To ensure logos remain visible and visually appealing in both light and dark themes, users need the ability to add custom logos for the masthead and favicon that adapt to the theme mode. To support this, a new API endpoint will be introduced, allowing users to specify different logos for light and dark themes.
Copy link

Choose a reason for hiding this comment

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

Suggested change
The existing OKD and OpenShift logos are designed for a dark masthead background and include white text, making them less effective in a light theme. To ensure logos remain visible and visually appealing in both light and dark themes, users need the ability to add custom logos for the masthead and favicon that adapt to the theme mode. To support this, a new API endpoint will be introduced, allowing users to specify different logos for light and dark themes.
The existing OKD and OpenShift logos are designed for a dark masthead background and include white text, making them ineffective in a light theme. To ensure logos remain visible and visually appealing in both light and dark themes, users need the ability to add custom logos for the masthead and favicon that adapt to the theme mode. To support this, a new API endpoint will be introduced, allowing users to specify different logos for light and dark themes.

I would replace less effective with ineffective.


1. Users could set both the `CustomLogoFile` and `CustomLogos` APIs. The `CustomLogos` API configuration will take precedence over the old `CustomLogoFile` field.

2. Each of the Console supported themes can be configured individually by setting either the Light or Dark theme type or by applying a default theme to all supported themes using the Default theme type. If the Default theme type is set along with a specific Dark or Light theme, the specific theme setting will override the default one.
Copy link

Choose a reason for hiding this comment

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

Suggested change
2. Each of the Console supported themes can be configured individually by setting either the Light or Dark theme type or by applying a default theme to all supported themes using the Default theme type. If the Default theme type is set along with a specific Dark or Light theme, the specific theme setting will override the default one.
2. Each of the Console supported themes can be configured individually by setting either the Light or Dark theme type, or by applying a default theme to all supported themes using the Default theme type. If the Default theme type is set along with a specific Dark or Light theme, the specific theme setting will override the default one.


2. Each of the Console supported themes can be configured individually by setting either the Light or Dark theme type or by applying a default theme to all supported themes using the Default theme type. If the Default theme type is set along with a specific Dark or Light theme, the specific theme setting will override the default one.

4. Users might experience confusion with the introduction of new logo configuration options, especially if they are familiar with using previous method, represented by the `CustomLogo` field, which will be deprecated. Provide a comprehensive documentation that guide users through the transition. Include clear instructions about the changes and their benefits.
Copy link

Choose a reason for hiding this comment

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

Should this be 3?


2. Each of the Console supported themes can be configured individually by setting either the Light or Dark theme type or by applying a default theme to all supported themes using the Default theme type. If the Default theme type is set along with a specific Dark or Light theme, the specific theme setting will override the default one.

4. Users might experience confusion with the introduction of new logo configuration options, especially if they are familiar with using previous method, represented by the `CustomLogo` field, which will be deprecated. Provide a comprehensive documentation that guide users through the transition. Include clear instructions about the changes and their benefits.
Copy link

Choose a reason for hiding this comment

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

Suggested change
4. Users might experience confusion with the introduction of new logo configuration options, especially if they are familiar with using previous method, represented by the `CustomLogo` field, which will be deprecated. Provide a comprehensive documentation that guide users through the transition. Include clear instructions about the changes and their benefits.
4. Users might experience confusion with the introduction of new logo configuration options. The prevoious method represented by `CustomLogo` will be deprecated. Provide comprehensive documentation that will guide users through the transition. Include clear instructions about the changes and their benefits.


#### Removing a deprecated feature

The current custom logo field in `customization.customLogo` is deprecated and will be removed at some point in the future. Users are encouraged to transition to the new custom logos configuration that supports light and dark theme modes for the masthead and favicon. The new custom logos feature also includes support for a default theme for all unspecified themes.
Copy link

Choose a reason for hiding this comment

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

Is it possible to say deprecated with a future release? I think it sounds a little more concrete than at some point in the future. It would make it consistent with what we use across the docs too.


#### Removing a deprecated feature

The current custom logo field in `customization.customLogo` is deprecated and will be removed at some point in the future. Users are encouraged to transition to the new custom logos configuration that supports light and dark theme modes for the masthead and favicon. The new custom logos feature also includes support for a default theme for all unspecified themes.
Copy link

Choose a reason for hiding this comment

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

Suggested change
The current custom logo field in `customization.customLogo` is deprecated and will be removed at some point in the future. Users are encouraged to transition to the new custom logos configuration that supports light and dark theme modes for the masthead and favicon. The new custom logos feature also includes support for a default theme for all unspecified themes.
The current custom logo field in `customization.customLogo` is deprecated and will be removed at some point in the future. Users are encouraged to transition to the new custom logos configuration that supports light and dark themes for the masthead and favicon. The new custom logos feature also includes support for a default theme for all unspecified themes.

@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch 2 times, most recently from c791085 to f75e6ab Compare February 21, 2025 13:37
@cajieh cajieh force-pushed the ability-to-add-second-logo-pt6-enhancements branch from f75e6ab to f913f00 Compare February 21, 2025 15:04
Copy link
Contributor

openshift-ci bot commented Feb 21, 2025

@cajieh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/markdownlint f913f00 link true /test markdownlint

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants