PageRegistration & BaseRegistry refactoring #1334

ixrock · 2020-11-11T15:43:35Z

Changes:

Registering menu items (ClusterManager/Cluster) moved away to separated registry from PageRegistry
Refactored BaseRegistry so now all extension pages registered under /extension/:name route prefix
Refactored TabLayout

Fix for hello-world example: https://github.com/lensapp/hello-world-extension

import { Component, Interface, LensRendererExtension } from "@k8slens/extensions";
import { ExampleIcon, ExamplePage } from "./page"
import React from "react"

export default class ExampleExtension extends LensRendererExtension {
  clusterPages = [
    {
      id: Symbol("example-extension"),
      components: {
        Page: () => <ExamplePage extension={this}/>,
      }
    }
  ]

  clusterPageMenus = [
    {
      id: this.clusterPages[0].id,
      title: "Hello World",
      components: {
        Icon: ExampleIcon
      }
    }
  ]
}

Signed-off-by: Roman <ixrock@gmail.com>

…_1258 # Conflicts: # src/extensions/lens-renderer-extension.ts

Signed-off-by: Roman <ixrock@gmail.com>

ixrock · 2020-11-11T15:51:02Z

package.json

@@ -241,6 +241,7 @@
    "openid-client": "^3.15.2",
    "path-to-regexp": "^6.1.0",
    "proper-lockfile": "^4.1.1",
+    "react-router": "^5.2.0",


This is moved to normal dependencies since required in runtime (matchPath is used in page-registry.ts and page-menu-registry.ts)

react apparently also required internally in react-router when used in runtime

Signed-off-by: Roman <ixrock@gmail.com>

jim-docker · 2020-11-11T16:39:49Z

extensions/support-page/main.ts

-      click() {
-        windowManager.navigate(supportPageURL());
+      click: () => {
+        windowManager.navigate(this.getPageUrl(pageUrl)); // todo: simplify


Did you see my comment here? Maybe this is similar to what you are already planning...

Page registration doesn't have ID

Registered page exists only in LensRendererExtension but access to URL might need from LensMainExtension as well (like from top-menu support item)

If even this possible should be done in another PR

Signed-off-by: Roman <ixrock@gmail.com>

jim-docker · 2020-11-11T18:19:16Z

extensions/support-page/renderer.tsx

@@ -20,9 +18,9 @@ export default class SupportPageRendererExtension extends LensRendererExtension
      item: (
        <div
          className="flex align-center gaps hover-highlight"
-          onClick={() => Navigation.navigate(supportPageURL())}
+          onClick={() => Navigation.navigate(this.getPageUrl(pageUrl))}


If you instead have a navigate() method on LensRendererExtension (or LensExtension?) then this could be

Suggested change

onClick={() => Navigation.navigate(this.getPageUrl(pageUrl))}

onClick={() => this.navigate(pageUrl)}

(the navigate() method would call this.getPageUrl(), no need to expose it. This aligns with what @jakolehm did in #1323

guess that didn't work out. Would be nice if it were possible ...

Worked in dev/local build :/

…_1258 # Conflicts: # extensions/support-page/main.ts # src/extensions/renderer-api/navigation.ts

Signed-off-by: Roman <ixrock@gmail.com>

jim-docker · 2020-11-11T22:38:02Z

docs/extensions/capabilities/common-capabilities.md

@@ -100,13 +100,22 @@ import { ExamplePage } from "./src/example-page"
 export default class ExampleRendererExtension extends LensRendererExtension {
  globalPages = [
    {
-      path: "/example-route",
-      hideInMenu: true,
+      routePath: "/items/:id?",


Seems cumbersome if the extension developer has to specify this. Can it default to this?

It's just an example. Now could be optional since we have route-prefix for all extensions.

jim-docker · 2020-11-11T22:42:07Z

docs/extensions/capabilities/common-capabilities.md

-      path: "/extension-example",
-      title: "Example Extension",
+      routePath: "/extension-example",
+      exact: true,


can default be true? So developer doesn't have to set it unless they want it false (which assumes they know what they are doing)

This is optional. Could be default whatever we want :)

jim-docker · 2020-11-11T22:44:57Z

extensions/support-page/renderer.tsx

@@ -20,9 +18,9 @@ export default class SupportPageRendererExtension extends LensRendererExtension
      item: (
        <div
          className="flex align-center gaps hover-highlight"
-          onClick={() => Navigation.navigate(supportPageURL())}
+          onClick={() => Navigation.navigate(this.getPageUrl(pageUrl))}


guess that didn't work out. Would be nice if it were possible ...

jim-docker · 2020-11-11T23:16:02Z

src/extensions/lens-main-extension.ts

  async navigate(location: string, frameId?: number) {
-    await WindowManager.getInstance<WindowManager>().navigate(location, frameId)
+    const windowManager = WindowManager.getInstance<WindowManager>();
+    const url = this.getPageUrl(location);
+    await windowManager.navigate(url, frameId)
  }


I like this a lot but @jakolehm couldn't get it to work?

I like this a lot but @jakolehm couldn't get it to work?

It works on main extension.

jim-docker · 2020-11-12T01:08:28Z

src/renderer/components/layout/sidebar.tsx

@@ -98,108 +97,100 @@ export class Sidebar extends React.Component<Props> {
          </div>
          <div className="sidebar-nav flex column box grow-fixed">
            <SidebarNavItem
-              id="cluster"


I think the integration tests rely on these ids

@ixrock pls revert removal of ids.

This is bad.

yes it's bad, but let's fix that in separate PR

jakolehm · 2020-11-12T07:52:45Z

src/renderer/components/layout/sidebar.tsx

@@ -98,108 +97,100 @@ export class Sidebar extends React.Component<Props> {
          </div>
          <div className="sidebar-nav flex column box grow-fixed">
            <SidebarNavItem
-              id="cluster"


@ixrock pls revert removal of ids.

nevalla · 2020-11-12T07:59:39Z

src/extensions/registries/base-registry.ts

-    })
+  remove(items: T[], key: LensExtension = null) {
+    const storedItems = this.items.get(key);
+    if (storedItems) {


could be also be

if (!storedItems) return;

…_1258 # Conflicts: # src/extensions/lens-renderer-extension.ts

Signed-off-by: Roman <ixrock@gmail.com>

…_1258 # Conflicts: # extensions/support-page/renderer.tsx

src/extensions/registries/page-menu-registry.ts

jakolehm · 2020-11-12T13:17:53Z

src/extensions/registries/page-registry.ts

-  subPages?: (PageRegistration & TabRoute)[];
+export interface PageRegistration extends BaseRegistryItem {
+  routePath?: string; // additional (suffix) route path to base extension's route: "/extension/:name"
+  exact?: boolean; // route matching flag, see: https://reactrouter.com/web/api/NavLink/exact-bool


This is a bit mystery flag ... when does it needs to be set?

I was hoping provided url from comment will explain this.. but.. it's about strict matching route:
e.g. if you have route like /page with exact: true it will match only this, but not /page/sub-page

Signed-off-by: Roman <ixrock@gmail.com>

nevalla · 2020-11-12T13:51:16Z

Something is off with the support icon/extension. Cannot click it.

ixrock · 2020-11-12T13:53:39Z

@nevalla you should recompile support-page extension (also got this after master-merge)
This is comes from #1331

ixrock added 4 commits November 10, 2020 17:24

PageRegistration refactoring #1258 -- part 1

9ec91c0

Signed-off-by: Roman <ixrock@gmail.com>

Merge remote-tracking branch 'origin/master' into page_registry_issue…

ab097c4

…_1258 # Conflicts: # src/extensions/lens-renderer-extension.ts

PageRegistration refactoring -- part 2

9b55a5f

Signed-off-by: Roman <ixrock@gmail.com>

fix: support-page extension

296ba90

Signed-off-by: Roman <ixrock@gmail.com>

ixrock requested review from jakolehm and a team November 11, 2020 15:43

jakolehm added area/extension Something to related to the extension api enhancement New feature or request labels Nov 11, 2020

jakolehm added this to the 4.0.0 milestone Nov 11, 2020

ixrock commented Nov 11, 2020

View reviewed changes

fix: make registry.add more backward compatible

e671d07

Signed-off-by: Roman <ixrock@gmail.com>

ixrock force-pushed the page_registry_issue_1258 branch from db77f8e to e671d07 Compare November 11, 2020 16:36

jim-docker reviewed Nov 11, 2020

View reviewed changes

ixrock added 2 commits November 11, 2020 19:17

update docs

824d102

Signed-off-by: Roman <ixrock@gmail.com>

fix build: react-router require react in runtime

451bc42

Signed-off-by: Roman <ixrock@gmail.com>

jim-docker reviewed Nov 11, 2020

View reviewed changes

ixrock added 2 commits November 12, 2020 00:33

Merge remote-tracking branch 'origin/master' into page_registry_issue…

0d16f61

…_1258 # Conflicts: # extensions/support-page/main.ts # src/extensions/renderer-api/navigation.ts

fixes

1823460

Signed-off-by: Roman <ixrock@gmail.com>

jim-docker reviewed Nov 12, 2020

View reviewed changes

Merge branch 'master' into page_registry_issue_1258

bc3a659

jakolehm requested changes Nov 12, 2020

View reviewed changes

nevalla reviewed Nov 12, 2020

View reviewed changes

ixrock added 4 commits November 12, 2020 11:52

Merge remote-tracking branch 'origin/master' into page_registry_issue…

6159ebf

…_1258 # Conflicts: # src/extensions/lens-renderer-extension.ts

more fixes & refactoring

a489d9a

Signed-off-by: Roman <ixrock@gmail.com>

support-page extension fixes

1fc8ff0

Signed-off-by: Roman <ixrock@gmail.com>

Merge remote-tracking branch 'origin/master' into page_registry_issue…

356cbef

…_1258 # Conflicts: # extensions/support-page/renderer.tsx

jakolehm reviewed Nov 12, 2020

View reviewed changes

fix integration tests + global page-registry interfaces

78a4f73

Signed-off-by: Roman <ixrock@gmail.com>

jakolehm approved these changes Nov 12, 2020

View reviewed changes

jakolehm merged commit faa1cef into master Nov 12, 2020

jakolehm deleted the page_registry_issue_1258 branch November 12, 2020 14:29

This was referenced Nov 12, 2020

Release v4.0.0-beta.1 #1351

Merged

Simplify pages/menus/registry extension api internal implementation #1364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PageRegistration & BaseRegistry refactoring #1334

PageRegistration & BaseRegistry refactoring #1334

ixrock commented Nov 11, 2020 •

edited

ixrock Nov 11, 2020 •

edited

ixrock Nov 11, 2020

jim-docker Nov 11, 2020

ixrock Nov 11, 2020 •

edited

jim-docker Nov 11, 2020

jim-docker Nov 11, 2020

jakolehm Nov 12, 2020

ixrock Nov 12, 2020

jim-docker Nov 11, 2020

ixrock Nov 12, 2020

jim-docker Nov 11, 2020

ixrock Nov 12, 2020

jim-docker Nov 11, 2020

jim-docker Nov 11, 2020

jakolehm Nov 12, 2020

jim-docker Nov 12, 2020

jakolehm Nov 12, 2020

ixrock Nov 12, 2020

nevalla Nov 12, 2020

jakolehm Nov 12, 2020

nevalla Nov 12, 2020

jakolehm Nov 12, 2020

ixrock Nov 12, 2020

nevalla commented Nov 12, 2020

ixrock commented Nov 12, 2020 •

edited

	onClick={() => Navigation.navigate(this.getPageUrl(pageUrl))}
	onClick={() => this.navigate(pageUrl)}

PageRegistration & BaseRegistry refactoring #1334

PageRegistration & BaseRegistry refactoring #1334

Conversation

ixrock commented Nov 11, 2020 • edited

ixrock Nov 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixrock Nov 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevalla commented Nov 12, 2020

ixrock commented Nov 12, 2020 • edited

ixrock commented Nov 11, 2020 •

edited

ixrock Nov 11, 2020 •

edited

ixrock Nov 11, 2020 •

edited

ixrock commented Nov 12, 2020 •

edited