-
Notifications
You must be signed in to change notification settings - Fork 116
feat(frontend) Support vendor branding #2695
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
base: master
Are you sure you want to change the base?
Conversation
- Moved vendor assets from src/assets to public/static/assets to make them replaceable with custom assets - GVendorIcon component serves icons from this directory using filenames from vendor information - Centralized dictionary for vendors (infra/dns/image) in config store - Added defaults for all known vendors (displayname, icon, weight) - All values can be extended or overridden via configuration - Removed need to provide createTitle and updateTile in GSecretDialog (now kebap-case generated from displayName) - Use vendorDisplayName everywhere to ensure we show configured name for vendors instead of technical name
klocke-io
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.
Looks a lot cleaner and simpler now 🎉
| | `displayName` | Name displayed in the dashboard | | ||
| | `weight` | Sorting weight. Lower values appear first. See default weights in `frontend/src/store/config.js` | | ||
| | `icon` | ile name of the icon located in the `public/static/assets` folder. See [Logos and Icons](#logos-and-icons) for instructions on replacing assets | | ||
|
|
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.
showing an example config file would be nice
|
@holgerkoser You have pull request review open invite, please check |
frontend/src/store/config.js
Outdated
| ...knownImageVendors, | ||
| ] | ||
|
|
||
| const vendorDetails = function (kind) { |
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.
do you think it makes sense to have a vendorDetailsMap computed prop? In order to compute it once? Something like the following
const vendorDetailsMap = computed(() => {
const map = new Map()
// Build map from all known and configured vendors
const allVendors = new Set([
...knownVendors.map(v => v.name),
...configVendors.value.map(v => v.name),
])
for (const kind of allVendors) {
const configuredVendor = find(configVendors.value, ['name', kind])
const knownVendor = find(knownVendors, ['name', kind])
map.set(kind, {
name: kind,
weight: Number.MAX_SAFE_INTEGER,
...knownVendor,
...configuredVendor,
})
}
return map
})
function vendorDetails (kind) {
return vendorDetailsMap.value.get(kind) ?? {
name: kind,
weight: Number.MAX_SAFE_INTEGER,
}
}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.
Yes. While working on your feedback i stumbled over that comment that there could be possible name clashes between the different vendor arrays. As they are already separated in config, I changed the code to include a type in the vendorDetails lookup.
As I did not want to adjust every component that uses the function to pass the param, I do this internally only and log a warning in case there are ambiguous or missing vendors. This, however slightly reduces the performance effect we gained by introducing the computed as we now have to loop over the types in the compatibility function. However this will change once we pass type from the components using the function
|
@grolu You need rebase this pull request with latest master branch. Please check. |
| function vendorDisplayName (name, type) { | ||
| return get(vendorDetails(name, type), ['displayName'], name) |
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.
vendorDetails only accepts one (name) param
| }, | ||
| ] | ||
|
|
||
| const knownImageVendors = [ |
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.
shouldn't we better name it to knownOSImageVendors or knownMachineImageVendors? Or do you think it is clear enough?
|
The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
You can:
/lifecycle stale |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1707 Fixes #207 (partly)
Special notes for your reviewer:
Open Questions:
TODO:
Adapt helm charts (values.yaml)
Release note: