Add optional rootWindow option to route plugin #5645

yaacov · 2020-06-02T07:02:57Z

Description
Add an optional option to render routes in root window instead of inside the layout.

Rational
Some plugin and currently internal views can benefit from "full screen" views.

Current use cases

For the kubevirt-plugin we want to render the VNC terminal using a detached window [1] [2] , having the possibility to add a "full screen" route in the plugin. will give us the option to maintaine the VNC console without the need to mange the hard coded route in the common app.
We currently have a /terminal path hard-coded to the app root paths, this approach can give developers the option to move the terminal view into a plugin structure if they will want to.

IMHO oc terminal view and VNC console view are just examples for a common need, maybe other users can think of other use cases like a "full screen" topology or monitoring views ?

Example
Route with rootWindow: true

  {
    type: 'Page/Route',
    properties: {
      exact: true,
      path: '/test-root',
      rootWindow: true,
      render: () => <h1>Test Root Window Page</h1>,
    },
  },

Screenshot

[1] RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1837677
[2] #5593

Signed-off-by: yaacov <kobi.zamir@gmail.com>

openshift-ci-robot · 2020-06-02T07:03:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yaacov
To complete the pull request process, please assign rawagner
You can assign the PR to them by writing /assign @rawagner in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yaacov · 2020-06-02T07:04:01Z

@spadgett @vojtechszocs @glekner please review

yaacov · 2020-06-02T08:56:35Z

/test e2e-gcp-console

glekner · 2020-06-02T11:46:08Z

frontend/packages/console-demo-plugin/src/plugin.tsx

@@ -187,6 +187,16 @@ const plugin: Plugin<ConsumedExtensions> = [
      },
    },
  },
+  {


Can you demonstrate this new functionality by migrating the /terminal route instead of creating a test route?

Nice 👍
For this PR I want to have smallest footprint, a simple route show how to use it while adding minimal code ... maybe we can do a variant of the VNC terminal PR that use this plugin option ?

Agreed with @glekner - if we're introducing a mechanism to support "full screen" pages (rendered outside the standard Console <App> layout) then we should also refactor existing code to utilize this mechanism for better consistency.

Right now, our React router has two top-level routes:

<Route path="/terminal" component={CloudShellTab} /> <Route path="/" component={App} />

@yaacov @spadgett @christianvogt Does it make sense to move "cloud shell" (<CloudShellExec> / xterm based CLI) functionality to one of the existing Console packages?

Yes let's convert the cloud shell to use this extension.

glekner

This is Great!

vojtechszocs · 2020-06-04T16:44:21Z

frontend/packages/console-demo-plugin/src/plugin.tsx

@@ -187,6 +187,16 @@ const plugin: Plugin<ConsumedExtensions> = [
      },
    },
  },
+  {


Agreed with @glekner - if we're introducing a mechanism to support "full screen" pages (rendered outside the standard Console <App> layout) then we should also refactor existing code to utilize this mechanism for better consistency.

Right now, our React router has two top-level routes:

<Route path="/terminal" component={CloudShellTab} /> <Route path="/" component={App} />

@yaacov @spadgett @christianvogt Does it make sense to move "cloud shell" (<CloudShellExec> / xterm based CLI) functionality to one of the existing Console packages?

vojtechszocs · 2020-06-04T16:45:51Z

frontend/packages/console-plugin-sdk/src/typings/pages.ts

@@ -63,6 +63,8 @@ namespace ExtensionProperties {
    path: string | string[];
    /** Perspective id to which this page belongs to. */
    perspective?: string;
+    /** rootWindow indicate rendering as root window instead of layout content. */


Suggested change

/** rootWindow indicate rendering as root window instead of layout content. */

/** Whether to render this page outside the standard Console page layout. */

vojtechszocs · 2020-06-04T16:51:11Z

frontend/public/components/app-contents.tsx

@@ -99,6 +100,15 @@ const getPluginPageRoutes = (activePerspective: string, flags: FlagsObject) =>
      return <Component {...r.properties} key={Array.from(r.properties.path).join(',')} />;
    });

+const getPluginRootWindowRoutes = () =>
+  plugins.registry.getRoutePages().map((r) => {


Suggested change

plugins.registry.getRoutePages().map((r) => {

plugins.registry.getRoutePages()

.filter((r) => r.properties?.rootWindow)

.map((r) => {

Let's move to useExtensions

vojtechszocs · 2020-06-04T16:52:04Z

frontend/public/components/app-contents.tsx

@@ -99,6 +100,15 @@ const getPluginPageRoutes = (activePerspective: string, flags: FlagsObject) =>
      return <Component {...r.properties} key={Array.from(r.properties.path).join(',')} />;
    });

+const getPluginRootWindowRoutes = () =>


Suggested change

const getPluginRootWindowRoutes = () =>

const getPluginTopLevelRoutes = () =>

vojtechszocs · 2020-06-04T16:53:21Z

frontend/packages/console-plugin-sdk/src/typings/pages.ts

@@ -63,6 +63,8 @@ namespace ExtensionProperties {
    path: string | string[];
    /** Perspective id to which this page belongs to. */
    perspective?: string;
+    /** rootWindow indicate rendering as root window instead of layout content. */
+    rootWindow?: boolean;


I think rootWindow is a bit ambiguous. Maybe noLayout ?

Honestly i don't think a concise variable name will be descriptive enough.
IMO I'd suggest root?: boolean as rootWindow doesn't provide any more description than the other.
OR to make it even more useful for other uses, we could do context?: string and let the consumer of the extension supply the context.

christianvogt · 2020-06-04T18:34:38Z

frontend/public/components/app-contents.tsx

@@ -99,6 +100,15 @@ const getPluginPageRoutes = (activePerspective: string, flags: FlagsObject) =>
      return <Component {...r.properties} key={Array.from(r.properties.path).join(',')} />;
    });

+const getPluginRootWindowRoutes = () =>
+  plugins.registry.getRoutePages().map((r) => {


Let's move to useExtensions

christianvogt · 2020-06-04T18:35:14Z

frontend/public/components/app.jsx

@@ -215,6 +215,7 @@ render(
  <Provider store={store}>
    <Router history={history} basename={window.SERVER_FLAGS.basePath}>
      <Switch>
+        {getPluginRootWindowRoutes()}
        <Route path="/terminal" component={CloudShellTab} />


Convert /terminal to use this new extension.

christianvogt · 2020-06-04T18:38:14Z

frontend/packages/console-plugin-sdk/src/typings/pages.ts

@@ -63,6 +63,8 @@ namespace ExtensionProperties {
    path: string | string[];
    /** Perspective id to which this page belongs to. */
    perspective?: string;
+    /** rootWindow indicate rendering as root window instead of layout content. */
+    rootWindow?: boolean;


Honestly i don't think a concise variable name will be descriptive enough.
IMO I'd suggest root?: boolean as rootWindow doesn't provide any more description than the other.
OR to make it even more useful for other uses, we could do context?: string and let the consumer of the extension supply the context.

christianvogt · 2020-06-04T18:38:34Z

frontend/packages/console-demo-plugin/src/plugin.tsx

@@ -187,6 +187,16 @@ const plugin: Plugin<ConsumedExtensions> = [
      },
    },
  },
+  {


Yes let's convert the cloud shell to use this extension.

openshift-ci-robot · 2020-06-13T18:57:06Z

@yaacov: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yaacov · 2020-08-04T12:09:48Z

This request was initiated to help with the VNC viewer PR, that is now low priority for us,
Closing since I do not have time to continue with this PR at this time.

Add optional rootWindow option to route plugin

331bfb4

Signed-off-by: yaacov <kobi.zamir@gmail.com>

openshift-ci-robot added the component/core Related to console core functionality label Jun 2, 2020

openshift-ci-robot requested review from spadgett and TheRealJon June 2, 2020 07:03

openshift-ci-robot added the component/sdk Related to console-plugin-sdk label Jun 2, 2020

glekner reviewed Jun 2, 2020

View reviewed changes

glekner suggested changes Jun 2, 2020

View reviewed changes

vojtechszocs suggested changes Jun 4, 2020

View reviewed changes

vojtechszocs mentioned this pull request Jun 4, 2020

open the VNC console in a new window #5593

Merged

christianvogt reviewed Jun 4, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2020

spadgett removed their request for review June 24, 2020 13:55

yaacov closed this Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional rootWindow option to route plugin #5645

Add optional rootWindow option to route plugin #5645

yaacov commented Jun 2, 2020 •

edited

openshift-ci-robot commented Jun 2, 2020

yaacov commented Jun 2, 2020

yaacov commented Jun 2, 2020

glekner Jun 2, 2020

yaacov Jun 2, 2020

vojtechszocs Jun 4, 2020

christianvogt Jun 4, 2020

glekner left a comment

vojtechszocs Jun 4, 2020

vojtechszocs Jun 4, 2020

vojtechszocs Jun 4, 2020

christianvogt Jun 4, 2020

vojtechszocs Jun 4, 2020

vojtechszocs Jun 4, 2020

christianvogt Jun 4, 2020

christianvogt Jun 4, 2020

christianvogt Jun 4, 2020

christianvogt Jun 4, 2020

christianvogt Jun 4, 2020

openshift-ci-robot commented Jun 13, 2020

yaacov commented Aug 4, 2020

@@ @@ -187,6 +187,16 @@ const plugin: Plugin<ConsumedExtensions> = [ @@
                     },
                   },
                 },
+                {

	/** rootWindow indicate rendering as root window instead of layout content. */
	/** Whether to render this page outside the standard Console page layout. */

	const getPluginRootWindowRoutes = () =>
	const getPluginTopLevelRoutes = () =>

Add optional rootWindow option to route plugin #5645

Add optional rootWindow option to route plugin #5645

Conversation

yaacov commented Jun 2, 2020 • edited

openshift-ci-robot commented Jun 2, 2020

yaacov commented Jun 2, 2020

yaacov commented Jun 2, 2020

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

glekner left a 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

openshift-ci-robot commented Jun 13, 2020

yaacov commented Aug 4, 2020

yaacov commented Jun 2, 2020 •

edited