-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Extension support page #1112
Extension support page #1112
Conversation
* WIP: Adding support page Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com> * lint Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com>
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
…upport_page # Conflicts: # extensions/telemetry/package.json # extensions/telemetry/renderer.tsx # extensions/telemetry/src/tracker.ts # src/extensions/core-api/registries.ts # src/extensions/extension-api.ts # src/extensions/extension-renderer-api.ts # src/extensions/lens-renderer-runtime.ts # src/extensions/renderer-api/registries.ts
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
for(const [id, ext] of installedExtensions) { | ||
let instance = this.instances.get(ext.manifestPath) | ||
for (const [id, ext] of installedExtensions) { | ||
let instance = this.instances.get(ext.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.
Without this change same extension might be enabled multiple times..
Possible reason: initial checking for ext.manifestPath
, but actual saving as installed-extensions happened via ext.id
key
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, I noticed this bug also in #1125.
@@ -34,7 +33,6 @@ export class LensExtension implements ExtensionModel { | |||
@observable manifest: ExtensionManifest; | |||
@observable manifestPath: string; | |||
@observable isEnabled = false; | |||
@observable.ref runtime: LensExtensionRuntimeEnv; |
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.
Removed initially planned "runtime". Now everything could be served via webpack's externals into global Lens runtime.
@@ -9,8 +9,8 @@ | |||
"styles": [] | |||
}, | |||
"scripts": { | |||
"build": "webpack --config webpack.config.js", | |||
"dev": "npm run build --watch" |
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.
For some reasons reusing command didn't work for me from WebStorm (maybe because need of $@
at the end or different running context, dunno). Btw webpack.config.js
is a default config name (even .ts
is supported, when used in conjunction with ts-node
).
import * as Registry from "../registries" | ||
import * as CommonVars from "../../common/vars"; | ||
|
||
export let windowManager: WindowManager; |
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.
Exported in runtime in src/main/index.ts
@@ -14,7 +14,7 @@ export interface ILanguage { | |||
|
|||
export const _i18n = setupI18n({ | |||
missing: (message, id) => { | |||
console.warn('Missing localization:', message, id); | |||
// console.warn('Missing localization:', message, id); |
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.
Turned off to avoid noise for now until we have officially i18n
supported.
this.disposers.push( | ||
registry.add({ | ||
parentId: "help", | ||
label: "Support", |
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.
this basically duplicates the "Community Slack" and "Report an Issue" Help submenu items that are already there...
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.
So need to remove from that place? What is the question / propose?
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.
Probably remove this "Support" menu item (although it's nice to demonstrate that this can be done). The existing menu items have the advantage of taking you directly to the link in one click, whereas with "Support" it's one click to get to the support page then another to get to the link.
export * from "./renderer-extension-api" | ||
// Extension-api types generation bundle (used by rollup.js) | ||
|
||
export * from "./core-api" |
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.
should this be main-api for consistency?
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's not only about main
. Currently this api also consumed in renderer process (whole file imported in bootstrap.tsx
).
Anyway I would split both apis with strong separation what's and where included later for more future-proof concept.
registerPages(registry: Registry.PageRegistry) { | ||
this.disposers.push( | ||
registry.add({ | ||
type: Registry.DynamicPageType.CLUSTER, | ||
type: Registry.PageRegistryType.CLUSTER, |
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.
I know it's out of scope for this PR but why can't LensRendererExtension
have something like:
registerPages(registry: Registry.PageRegistry) {
for (let page of this.pages()) {
this.disposers.push(registry.add(page))
}
and then the subclasses just have to implement pages()
which returns an array of the pages to add?
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.
This API definitely will be improved to git rid of everything redundant for end user of the extensions.
logger.info('[EXTENSIONS-LOADER]: load on main renderer') | ||
this.autoloadExtensions(getLensRuntimeEnv, (instance: LensRendererExtension) => { | ||
loadOnClusterManagerRenderer() { | ||
logger.info('[EXTENSIONS-LOADER]: load on main renderer (cluster manager)') |
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.
what does main renderer
mean?
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.
Main renderer (term added not by me) means it's a top renderer for main BrowserWindow
when app starts and "not-main" renderer it's a renderer process(es) (child renderer process in iframe) where dashboard for single cluster is running.
this.autoloadExtensions(getLensRuntimeEnv, (instance: LensRendererExtension) => { | ||
loadOnClusterManagerRenderer() { | ||
logger.info('[EXTENSIONS-LOADER]: load on main renderer (cluster manager)') | ||
this.autoloadExtensions((instance: LensRendererExtension) => { | ||
instance.registerPages(pageRegistry) |
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.
should this line be removed from here since it is added to loadOnClusterRenderer()
?
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.
Nope, it's registering pages in different contexts (cluster-manager and dashboard / single cluster)
} | ||
|
||
@observer | ||
export class PageLayout extends React.Component<PageLayoutProps> { |
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.
nice
…upport_page # Conflicts: # package.json # src/extensions/core-api/registries.ts # src/extensions/extension-loader.ts # src/extensions/lens-renderer-extension.ts # src/extensions/renderer-api/components.ts
Signed-off-by: Roman <ixrock@gmail.com>
@jakolehm can this be merged already? |
for(const [id, ext] of installedExtensions) { | ||
let instance = this.instances.get(ext.manifestPath) | ||
for (const [id, ext] of installedExtensions) { | ||
let instance = this.instances.get(ext.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.
Yeah, I noticed this bug also in #1125.
import { BaseRegistry } from "./base-registry"; | ||
import { computed } from "mobx"; | ||
|
||
export enum PageRegistryType { |
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 feels that we should have separate APIs for each type (just to be more futureproof). Ofc that should be done in a separate PR.
Add link to support page on bottom bar, close #742
PR's most noticeable changes:
Help -> Support
)/support
material-icons-font
from master repo (npm
doesn't contain the latest icons pack, e.g.support
icon)Tooltip
component fixes: previously didn't work in some case (e.g. small icon in the screen corner with bigger tooltip)PageLayout
new common component: used forSupport
,Preferences
andClusterSettings
pagesScreenshots