add Perspective extension and default admin perspective #1615

christianvogt · 2019-05-25T20:11:30Z

Adds a new extension for contributing a Perspective. This is for supporting the developer perspective.

  export interface Perspective {
    id: string;
    name: string;
    icon: React.ReactElement;
    landingPageURL: string;
    default?: boolean;
  }

Changes includes:

Contribution of default Administrator perspective.
Ability to switch perspectives from the application launcher menu in the masthead as a temporary solution until PF4 and UX settle on a design for the mega-menu. Unless there are multiple perspectives registered, the actions will not show up in this menu. Therefore until we have the developer perspective merged into the repo, there won't be able visible UI related to perspectives.
Adds redux actions, reducers, and selectors for working with perspectives.
When the perspective is set, its ID is stored in localStorage such that a returning user will be in their last active perspective. This functionality is similar to active namespace.

fyi @rohitkrai03

christianvogt · 2019-05-25T20:12:37Z

/cc @vojtechszocs @spadgett

christianvogt · 2019-05-25T23:25:52Z

/retest

spadgett

Looks good. Will need rebase.

@vojtechszocs PTAL

/approve

spadgett · 2019-05-28T14:40:40Z

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

+    /* The perspective display icon. */
+    icon: React.ReactNode;
+    /* The perspective landing page URL. */
+    landingPageUrl: string;


nit: with current console naming conventions, this would be URL

Suggested change

landingPageUrl: string;

landingPageURL: string;

Updated to landingPageURL

spadgett · 2019-05-28T14:43:56Z

frontend/public/components/masthead-toolbar.jsx

+      ...perspectives
+        .sort((a, b) => a.properties.name.localeCompare(b.properties.name))
+        .map(p => ({
+          label: <span>{p.properties.icon} {p.properties.name}</span>,


React.Fragment?

Changed to use React.Fragment

vojtechszocs

@christianvogt This looks great! Please see my inline comments.

vojtechszocs · 2019-05-28T16:14:33Z

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

@@ -0,0 +1,25 @@
+


Nit-pick: please remove leading newline.

vojtechszocs · 2019-05-28T16:19:17Z

frontend/packages/console-app/package.json

@@ -8,5 +8,8 @@
    "@console/internal": "0.0.0-fixed",
    "@console/plugin-sdk": "0.0.0-fixed",
    "@console/shared": "0.0.0-fixed"
+  },
+  "consolePlugin": {


This means that Console application itself can contribute extensions, I like that 👍

As a follow-up, we can revisit supported extension types and represent related Console application code through console-app/src/plugin.tsx (e.g. all the base models).

This also means we'll likely need a way to order plugins before processing their extensions, i.e. console-app's models should be defined before any other models.

I was thinking about e.g. "order": "before:X" and "order": "after:X" within the consolePlugin object, where X is the name of the plugin or after:@console/app (if not specified, it would fallback to after:all).

I can see value in having order but not for the current set of use cases we have.
If we can avoid introducing an order altogether for extensions I think it would be great to keep things simple.

vojtechszocs · 2019-05-28T16:31:51Z

frontend/packages/console-plugin-sdk/src/codegen/__tests__/index.spec.ts

@@ -69,6 +69,17 @@ describe('codegen', () => {
        { ...pluginPackages[0] },
      ]);
    });
+
+    it('should include appPackage as a valid plugin package', () => {


👍 on test-covering the change in getActivePluginPackages

vojtechszocs · 2019-05-28T16:38:59Z

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

+    icon: React.ReactNode;
+    /* The perspective landing page URL. */
+    landingPageURL: string;
+    /* Whether the perspective is the default. There can only be one default. */


What happens if:

a plugin declares two perspectives each with default: true

two plugins each declare a perspective with default: true

This could be handled by taking the first default one and discarding the rest, or just omitting the default option and assume there will always be at least one perspective (added by Console application itself) and that one is the default (because it's declared as the first one).

@christianvogt @spadgett What are your thoughts on this?

This relates to my comment above, regarding plugin evaluation order, i.e. Console application's plugin should be processed before any other plugins.

Also, what happens if a plugin tries to redefine existing perspective (same id)?

I think there should be a generic mechanism for each extension owner (if we ever support plugins providing their own extensions then we'll have more than console core owning extensions) to define their own extension processor. This would allow the owner to validate the contributed extensions. Duplicate perspective IDs or multiple defined as a default would be runtime errors.

Maybe we should create an ExtensionProcessor extension :) wherein you contribute a function to process extensions by type. The function would receive the list of contributed extensions and it could throw an error in case of duplicate IDs. It could also log warnings. The return value is an array of extensions in case the processor chose to add or subtract from the list. This mechanism could be used to contribute CRD based extensions in the future. eg a mega-menu extension will get extra values contributed via CRDs.

vojtechszocs · 2019-05-28T16:51:45Z

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

+} from '@console/plugin-sdk';
+
+type ConsumedExtensions =
+  | Perspective;


Do we want to add a Perspective extension example to console-demo-plugin too?

(I was thinking of a unit test that checks if demo plugin uses all supported extension types.)

Added a sample perspective.

christianvogt · 2019-05-28T18:57:19Z

/retest

christianvogt · 2019-05-29T04:02:50Z

@vojtechszocs @spadgett I've updated this PR with the suggested changes.

spadgett

/lgtm

openshift-ci-robot · 2019-05-29T15:40:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, spadgett

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

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

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 25, 2019

openshift-ci-robot requested review from jeff-phillips-18 and jhadvig May 25, 2019 20:11

openshift-ci-robot requested review from spadgett and vojtechszocs May 25, 2019 20:12

christianvogt force-pushed the perspectives branch from 0a9bd5e to 81e3503 Compare May 25, 2019 22:12

spadgett added this to the v4.2 milestone May 27, 2019

spadgett approved these changes May 28, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2019

christianvogt force-pushed the perspectives branch 2 times, most recently from 81de0c5 to a66f1a5 Compare May 28, 2019 15:14

vojtechszocs suggested changes May 28, 2019

View reviewed changes

christianvogt force-pushed the perspectives branch from a66f1a5 to dc6e54a Compare May 28, 2019 17:32

christianvogt force-pushed the perspectives branch 4 times, most recently from 0c34c36 to 3610511 Compare May 29, 2019 01:50

add Perspective extension and default admin perspective

1fea925

christianvogt force-pushed the perspectives branch from 3610511 to 1fea925 Compare May 29, 2019 04:29

openshift-ci-robot assigned spadgett May 29, 2019

spadgett approved these changes May 29, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2019

openshift-merge-robot merged commit ec53265 into openshift:master May 29, 2019

christianvogt deleted the perspectives branch June 5, 2019 13:35

add Perspective extension and default admin perspective #1615

add Perspective extension and default admin perspective #1615

Uh oh!

Conversation

christianvogt commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christianvogt commented May 25, 2019

Uh oh!

christianvogt commented May 25, 2019

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtechszocs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christianvogt commented May 28, 2019

Uh oh!

christianvogt commented May 29, 2019

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christianvogt commented May 25, 2019 •

edited

Loading