-
Notifications
You must be signed in to change notification settings - Fork 585
Add Custom product name and logo to api #304
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
Add Custom product name and logo to api #304
Conversation
|
/retest |
|
/retest |
|
/retest |
|
/retest |
soltysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, now this looks reasonable.
f80b650 to
9880402
Compare
| // - ConfigMap.binaryData["filename"] - logo in binary form. | ||
| // This is best generated using: oc create configmap my-branding-config --from-file=/path/to/file -n openshift-config | ||
| // Image size must be less than 1 MB due to constraints on configmap size | ||
| // +optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
// customLogoFile replaces the default OpenShift logo in the masthead and about dialog. It is a reference to a
// ConfigMap in the openshift-config namespace. This can be created with a command like
// 'oc create configmap custom-logo --from-file=/path/to/logo.svg -n openshift-config'.
// Image size must be less than 1 MB due to constraints on the ConfigMap size.
// The ConfigMap key should include a file extension so that the console serves the file
// with the correct MIME type.
// +optionalThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to give guidance on recommended dimensions (although I don't know what those are offhand).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we set the viewbox to 100x100 on the SVGs. Should I add that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spadgett
// Recommended logo is of type svg or logo size of 100px x 100px
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100x100 can't be right since the logo is not square.
Maybe we simply say "An SVG logo is recommended, but other image types are supported." and leave it at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we should indicate a recommended size, along with svg so its scalable. Our own logos:
https://github.com/openshift/console/blob/master/frontend/public/imgs/openshift-dedicated-logo.svg
https://github.com/openshift/console/blob/master/frontend/public/imgs/openshift-online-logo.svg
Are 40px high, but width is variable, roughly 260px in browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zherman0 the PatternFly variable --pf-c-page__header-brand-link--c-brand--MaxHeight sets a max-height: 3.75rem which is approximately 68px. So if the image is larger than 68px it will scale down proportionally.
I would say
// Recommended logo specifications:
// Dimensions: Max height of 68px and max width of 200px
// SVG format preferred
9880402 to
97d6c43
Compare
65386b6 to
4e53529
Compare
spadgett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh, spadgett, zherman0 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Update the operator types for console to include a customizable product name and logo file