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

Extension support page #1112

Merged
merged 13 commits into from
Oct 24, 2020
Merged

Extension support page #1112

merged 13 commits into from
Oct 24, 2020

Conversation

ixrock
Copy link
Member

@ixrock ixrock commented Oct 22, 2020

Add link to support page on bottom bar, close #742

PR's most noticeable changes:

  • Support page extension:
    • adds top menu item (Help -> Support)
    • adds status-bar support-ring icon (app's bottom-right corner)
    • adds global page accessible from /support
  • switching to manual updates for 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 for Support, Preferences and ClusterSettings pages

Screenshots

Screenshot 2020-10-22 at 13 19 46

Screenshot 2020-10-22 at 13 20 18

jim-docker and others added 9 commits October 19, 2020 14:28
* 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>
…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>
@ixrock ixrock added the area/extension Something to related to the extension api label Oct 22, 2020
@ixrock ixrock added this to the 3.7.0 milestone Oct 22, 2020
@ixrock ixrock requested review from nevalla, jakolehm and a team October 22, 2020 10:19
@ixrock ixrock changed the base branch from master to extensions-api October 22, 2020 10:25
@ixrock ixrock changed the title [WIP]: Extension support page Extension support page Oct 22, 2020
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)
Copy link
Member Author

@ixrock ixrock Oct 22, 2020

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

Copy link
Contributor

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;
Copy link
Member Author

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"
Copy link
Member Author

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;
Copy link
Member Author

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);
Copy link
Member Author

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",
Copy link
Contributor

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...

Copy link
Member Author

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?

Copy link
Contributor

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"
Copy link
Contributor

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?

Copy link
Member Author

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.

Comment on lines 10 to +13
registerPages(registry: Registry.PageRegistry) {
this.disposers.push(
registry.add({
type: Registry.DynamicPageType.CLUSTER,
type: Registry.PageRegistryType.CLUSTER,
Copy link
Contributor

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?

Copy link
Member Author

@ixrock ixrock Oct 23, 2020

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)')
Copy link
Contributor

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?

Copy link
Member Author

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)
Copy link
Contributor

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()?

Copy link
Member Author

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> {
Copy link
Contributor

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>
@ixrock
Copy link
Member Author

ixrock commented Oct 23, 2020

@jakolehm can this be merged already?

@ixrock ixrock requested a review from a team October 24, 2020 03:50
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)
Copy link
Contributor

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 {
Copy link
Contributor

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.

@jakolehm jakolehm merged commit f3a0059 into extensions-api Oct 24, 2020
@jakolehm jakolehm deleted the extension_support_page branch October 24, 2020 06:24
@nevalla nevalla modified the milestones: 3.7.0, 4.0.0 Oct 26, 2020
@jakolehm jakolehm mentioned this pull request Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extension Something to related to the extension api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add link to support page on bottom bar
4 participants