Feat(cli-terminal): Create cli-terminal #4762

abhinandan13jan · 2020-03-18T15:32:52Z

Feature:

https://issues.redhat.com/browse/ODC-3055

Analysis / Root cause:

Create Cloudshell Terminal Component, Icon & Feature Flag it

Solution Description:

Creates a set of components in console/app to create and launch a Terminal component integrated to Workspace component to start a cloudshell session.
Also, feature flags the whole setup to the presence of DevWorkspace CRD

Current Namespace Setting
Currently implementation creates and reads Workspace resource from dedicated namespace che-controller-operator, which needs to be updated as per final discussion.

ScreenShots

( might take time to load )...

Unit tests:

packages/console-app/src/components/cloud-shell/tests/cloudshell-resource.spec.ts
packages/console-app/src/components/cloud-shell/tests/CloudShellTerminalFrame.spec.tsx

Browser conformance:

Chrome 73

How to Review:

After pulling the code changes to local follow instructions at

https://github.com/che-incubator/che-workspace-operator

**Step 2: Add Icon **
TerminalIcon is commented out in masthead-toolbar... Bring the icon in by removing the comments

andrewballantyne · 2020-03-19T18:20:34Z

/kind feature

sahil143

I know there is still some work left to be done, But few redux comments that need attention. Also Need proper typings.

sahil143 · 2020-03-26T15:25:32Z

frontend/packages/console-app/src/components/terminal/Terminal.tsx

+  toggleTerminal,
+}) => (
+  <CloudShellDrawer open={isTerminalExpanded} onClose={() => toggleTerminal()}>
+    <CloudShellBody />


Remove this and use CloudShellTerminal here. This was just a dummy component

sahil143 · 2020-03-26T15:33:49Z

frontend/packages/console-app/src/components/terminal/Terminal.tsx

+);
+
+const terminalStateToProps = ({ UI }: RootState) => ({
+  isTerminalExpanded: !!UI.getIn(['terminal', 'isExpanded']),


Why double !!? isExpanded should be a boolean value or have open, close state.

sahil143 · 2020-03-26T15:38:33Z

frontend/packages/console-app/src/components/terminal/Terminal.tsx

+const connecttoTerminalIcon = connect((state: RootState) => terminalStateToProps(state), {
+  toggleTerminal: UIActions.terminalDrawerToggleExpanded,
+});
+const ConnectedTerminal = connecttoTerminalIcon(TerminalContent);
+
+export default ConnectedTerminal;


Suggested change

const connecttoTerminalIcon = connect((state: RootState) => terminalStateToProps(state), {

toggleTerminal: UIActions.terminalDrawerToggleExpanded,

});

const ConnectedTerminal = connecttoTerminalIcon(TerminalContent);

export default ConnectedTerminal;

const terminalPropsToState = {

toggleTerminal: UIActions.terminalDrawerToggleExpanded,

};

export default connect(terminalStateToProps, terminalPropsToState)(TerminalContent);

sahil143 · 2020-03-26T15:43:11Z

frontend/packages/console-app/src/components/terminal/redux/reducer.ts

+export default (state: State, action: TerminalAction) => {
+  if (!state) {
+    return Map({
+      terminal: () => {},


currently your state looks like this

temrinal: { terminal: () => {} }

change

Suggested change

terminal: () => {},

isExpanded: false,

sahil143 · 2020-03-26T15:44:38Z

frontend/packages/console-app/src/components/terminal/redux/reducer.ts

+    });
+  }
+  if (action.type === Actions.TerminalDrawerToggleExpanded)
+    return state.setIn(['terminal', 'isExpanded'], !state.getIn(['terminal', 'isExpanded']));


change this according to the above state.

sahil143 · 2020-03-26T15:46:43Z

frontend/packages/console-app/src/components/terminal/Terminal.tsx

+  toggleTerminal: any;
+};
+
+const TerminalContent: React.FC<TerminalContentProps> = ({


Rename this to CloudShell

sahil143 · 2020-03-26T15:48:09Z

frontend/packages/console-app/src/components/terminal/TerminalToolicon.tsx

+  flags: Record<string, any>;
+  terminalToggle: any;
+};
+const TerminalToolicon: React.FC<TerminalTooliconProps> = ({ flags, terminalToggle }) => {


Rename CloudShellIcon

sahil143 · 2020-03-26T15:48:57Z

frontend/packages/console-app/src/components/terminal/TerminalToolicon.tsx

+const ConnectedTerminalToolicon = connectToFlags(FLAG_DEVWORKSPACE)(TerminalToolicon);
+
+export default ConnectedTerminalToolicon;


Suggested change

const ConnectedTerminalToolicon = connectToFlags(FLAG_DEVWORKSPACE)(TerminalToolicon);

export default ConnectedTerminalToolicon;

export default connectToFlags(FLAG_DEVWORKSPACE)(TerminalToolicon);

sahil143

@abhinandan13jan The background should be black even in the loading state and your loading state is different from the one in the UX designs.

sahil143 · 2020-03-27T18:40:31Z

frontend/packages/console-app/src/components/cloud-shell/CloudShell.tsx

-  );
+type CloudshellContentProps = {
+  isTerminalExpanded: boolean;
+  toggleTerminal: any;


remove any, type should be () => void

sahil143 · 2020-03-27T18:42:58Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellIcon.tsx

+type terminalToggle = () => void;
+
+type TerminalTooliconProps = {
+  flags: Record<string, any>;


add proper type in place of any

sahil143 · 2020-03-27T18:44:21Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellIcon.tsx

+
+type TerminalTooliconProps = {
+  flags: Record<string, any>;
+  terminalToggle: terminalToggle;


terminalToggle is not reusable type so use directly () => void

sahil143 · 2020-03-27T18:47:25Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellIcon.tsx

+const TerminalToolicon: React.FC<TerminalTooliconProps> = ({ flags, terminalToggle }) => {
+  if (!flags[FLAG_DEVWORKSPACE]) return null;
+  return (
+    <ToolbarItem>
+      <Button variant="plain" aria-label="Terminal" onClick={terminalToggle}>
+        <TerminalIcon className="co-masthead-icon" />
+      </Button>
+    </ToolbarItem>
+  );
+};


can be simplified

Suggested change

const TerminalToolicon: React.FC<TerminalTooliconProps> = ({ flags, terminalToggle }) => {

if (!flags[FLAG_DEVWORKSPACE]) return null;

return (

<ToolbarItem>

<Button variant="plain" aria-label="Terminal" onClick={terminalToggle}>

<TerminalIcon className="co-masthead-icon" />

</Button>

</ToolbarItem>

);

};

const TerminalToolicon: React.FC<TerminalTooliconProps> = ({ flags, terminalToggle }) => ( flags[FLAG_DEVWORKSPACE] ? (

<ToolbarItem>

<Button variant="plain" aria-label="Terminal" onClick={terminalToggle}>

<TerminalIcon className="co-masthead-icon" />

</Button>

</ToolbarItem>

) : null );

sahil143 · 2020-03-27T18:48:10Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellIcon.tsx

+  if (!flags[FLAG_DEVWORKSPACE]) return null;
+  return (
+    <ToolbarItem>
+      <Button variant="plain" aria-label="Terminal" onClick={terminalToggle}>


aria-label should be Command Line Terminal and there should be tooltip for this button. Please, check the UX design doc for the content of tooltip.

sahil143 · 2020-03-27T19:21:04Z

frontend/packages/console-app/src/components/cloud-shell/redux/reducer.ts

+export default (state: State, action: TerminalAction) => {
+  if (!state) {
+    return Map({
+      terminal: { isExpanded: false },


This should be

Suggested change

terminal: { isExpanded: false },

isExpanded: false,

because you already have terminal property in the combineReducer.

sahil143 · 2020-03-27T19:23:30Z

frontend/packages/console-app/src/components/cloud-shell/redux/reducer.ts

+    });
+  }
+  if (action.type === Actions.TerminalDrawerToggleExpanded)
+    return state.setIn(['terminal', 'isExpanded'], !state.getIn(['terminal', 'isExpanded']));


we are using cloud-shell name everywhere. We should look to rename terminal to cloudShell here as well.

sahil143 · 2020-03-27T19:25:46Z

frontend/packages/console-app/src/components/cloud-shell/utils/cloudshell-resource.ts

+  value: string;
+  name: string;
+};
+type Component = {


Component is very generic name. It would be good idea to rename this to something else like DevFileComponents.

sahil143 · 2020-03-27T19:35:02Z

frontend/packages/console-app/src/components/cloud-shell/CloudShell.tsx

+  isTerminalExpanded,
+  toggleTerminal,
+}) => (
+  <CloudShellDrawer open={isTerminalExpanded} onClose={() => toggleTerminal()}>


There should be a confirmation modal before closing the cloud shell.

sahil143 · 2020-03-27T19:40:04Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellIcon.tsx

+  terminalToggle: terminalToggle;
+};
+
+const TerminalToolicon: React.FC<TerminalTooliconProps> = ({ flags, terminalToggle }) => {


rename this to CloudShellIcon

sahil143 · 2020-03-30T20:00:08Z

frontend/packages/console-app/src/components/cloud-shell/CloudShell.tsx

+
+const dispatchToProps = (dispatch: Dispatch): DispatchProps => ({
+  onClose: () =>
+    confirmModal({


This should not be part of redux callbacks, create a sperate callback for modal something like this

const closeCloudShellConfirmationModal = (callback) => {confirmModal({ title: 'Close Terminal?', message: ( <> This will close the terminal session. Content in the terminal will not be resolved in the next seesion. </> ), btnText: 'Yes', submitDanger: true, cancelText: 'No', executeFn: callback, })};

you can use the same in CloudShellMastheadButton as well.

updated this

sahil143 · 2020-03-30T20:01:40Z

frontend/packages/console-app/src/components/cloud-shell/CloudShell.tsx

-        <CloudShellBody />
-      </CloudShellDrawer>
-    </>
+    <CloudShellDrawer open={open} onClose={onClose}>


Suggested change

<CloudShellDrawer open={open} onClose={onClose}>

<CloudShellDrawer open={open} onClose={() => closeTerminalConfirmationModal(onClose)}>

sahil143 · 2020-03-30T20:11:45Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellDrawer.tsx

-export type CloudShellDrawerProps = {
-  open: boolean;
+type CloudShellDrawerProps = {
+  open?: boolean;


Not sure, why this is optional. We don't provide a default value for open also if this is omitted from the props then we would have no way to render the drawer component.

sahil143 · 2020-03-30T20:13:37Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellMastheadButton.tsx

+
+  const toggleTerminal = () => {
+    if (open) {
+      return confirmModal({


use a common utility for this, same as above closeCloudShellConfirmationModal

sahil143 · 2020-03-30T20:17:28Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellMastheadButton.tsx

+  }
+
+  return (
+    <Tooltip content={open ? 'Close Terminal' : 'Open Terminal'}>


Tooltip content Open command line terminal. Check design doc.
Also, the tooltip should be on the button instead of ToolbarItem.

sahil143 · 2020-03-30T20:29:19Z

frontend/public/components/app.jsx

@@ -2,10 +2,12 @@ import * as _ from 'lodash-es';
 import * as React from 'react';
 import { render } from 'react-dom';
 import { Helmet } from 'react-helmet';
+


remove this extra line

parvathyvr · 2020-03-31T11:59:46Z

@abhinandan13jan From the GIF i see that the 'Close' modal message is not correct and there is a typo in the 2nd line for the word 'session'...Please refer the design doc for the proposed message in the modal-https://docs.google.com/document/d/1UsyTYm67FXtcprku6F3L0WD48FjwTlgpDnC5Dj9kUGg/edit?ts=5df25614#heading=h.yy8m94syjcx2

abhinandan13jan · 2020-03-31T12:16:31Z

@parvathyvr I've updated the message to this

      <>
        This will close the terminal session. Content in the terminal will not be restored on
        next session.
      </>

frontend/packages/console-app/src/components/cloud-shell/CloudShell.tsx

christianvogt · 2020-04-01T15:43:43Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellConfirmationModal.tsx

+    message: (
+      <>
+        This will close the terminal session. Content in the terminal will not be restored on next
+        session.
+      </>
+    ),


Suggested change

message: (

<>

This will close the terminal session. Content in the terminal will not be restored on next

session.

</>

),

message: 'This will close the terminal session. Content in the terminal will not be restored on next session.',

With this change, remove the React import and rename file to use .ts suffix.

christianvogt · 2020-04-01T15:47:30Z

frontend/packages/console-app/src/components/cloud-shell/__tests__/CloudShellDrawer.spec.tsx

@@ -40,7 +40,7 @@ describe('CloudShellDrawerComponent', () => {
      .find(Drawer)
      .shallow()
      .find(Button);
-    expect(buttons.at(1).props()['aria-label']).toEqual('Close Terminal');
+    expect(buttons.at(1).props()['aria-label']).toEqual('Close terminal');


This is where having a data-test-id would be beneficial.

using data-test-id

christianvogt · 2020-04-01T15:50:07Z

frontend/public/components/masthead-toolbar.jsx

@@ -572,6 +573,7 @@ class MastheadToolbarContents_ extends React.Component {
                <PlusCircleIcon className="co-masthead-icon" />
              </Button>
            </ToolbarItem>
+            <CloudShellMastheadButton />


Let's comment this out before merging because we need to fix the workspace namespace and resource name before it can be used.

commented out

christianvogt · 2020-04-01T15:51:54Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellConfirmationModal.tsx

+  });
+};
+
+export default cloudshellConfirmationModal;


Rename file to not be uppercase because this does not export a react component.

abhinandan13jan · 2020-04-02T12:27:04Z

/test e2e-gcp-console

christianvogt · 2020-04-02T17:21:18Z

frontend/packages/console-app/src/components/cloud-shell/CloudshellTerminal.tsx

+  );
+};
+
+export default CloudShellTerminal;


This file is incorrectly named CloudshellTerminal.tsx where it should be named CloudShellTerminal.tsx

abhinandan13jan · 2020-04-06T07:50:17Z

/test e2e-gcp-console

sahil143

@abhinandan13jan cloud shell is not minimizing properly

abhinandan13jan · 2020-04-06T08:46:33Z

@abhinandan13jan cloud shell is not minimizing properly

I wasnt able to see them, when I last tested....Let's take these as bugs related to CSS... as we have lot of Pending work in the epic...
It is crucial to get this PR in as we have dependant stories waiting. @sahil143

sahil143 · 2020-04-06T08:53:08Z

/lgtm

I wasnt able to see them, when I last tested....Let's take these as bugs related to CSS... as we have lot of Pending work in the epic...
It is crucial to get this PR in as we have dependant stories waiting. @sahil143

Please raise a Jira ticket for this

abhinandan13jan · 2020-04-06T09:09:50Z

https://issues.redhat.com/browse/ODC-3495 Added this for CSS refinement

christianvogt · 2020-04-06T17:06:53Z

/approve

openshift-ci-robot · 2020-04-06T17:07:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, christianvogt, sahil143

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

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

openshift-bot · 2020-04-06T18:05:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-06T20:02:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-06T20:28:14Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2020

openshift-ci-robot requested review from benjaminapetersen and rhamilto March 18, 2020 15:33

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

openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2020

abhinandan13jan force-pushed the terminal branch 6 times, most recently from d188f87 to 5e91d37 Compare March 26, 2020 15:16

abhinandan13jan changed the title ~~WIP: feat(cli-terminal): Create cli-terminal~~ Feat(cli-terminal): Create cli-terminal Mar 26, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2020

sahil143 reviewed Mar 26, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal branch from 5e91d37 to 9881603 Compare March 27, 2020 15:40

sahil143 reviewed Mar 27, 2020

View reviewed changes

openshift-ci-robot added component/olm Related to OLM component/shared Related to console-shared labels Mar 30, 2020

abhinandan13jan force-pushed the terminal branch from 30fb59a to d97e613 Compare March 30, 2020 14:08

sahil143 reviewed Mar 30, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal branch from d97e613 to 6c06050 Compare March 31, 2020 11:13

abhinandan13jan force-pushed the terminal branch from 6c06050 to a9c1e0f Compare March 31, 2020 12:11

abhinandan13jan force-pushed the terminal branch from a9c1e0f to a5aabb8 Compare March 31, 2020 12:55

sahil143 reviewed Mar 31, 2020

View reviewed changes

frontend/packages/console-app/src/components/cloud-shell/CloudShell.tsx Outdated Show resolved Hide resolved

christianvogt reviewed Apr 1, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal branch 2 times, most recently from cb73eff to 4f6a624 Compare April 2, 2020 10:01

christianvogt reviewed Apr 2, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal branch from 4f6a624 to 8682b77 Compare April 3, 2020 10:18

feat(cli-terminal): Create cli-terminal

14e0cd4

abhinandan13jan force-pushed the terminal branch from 8682b77 to 14e0cd4 Compare April 3, 2020 16:32

sahil143 reviewed Apr 6, 2020

View reviewed changes

openshift-ci-robot assigned sahil143 Apr 6, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2020

openshift-merge-robot merged commit 4bbaa7e into openshift:master Apr 7, 2020

spadgett added this to the v4.5 milestone Apr 7, 2020

atiratree mentioned this pull request May 25, 2020

Bug 1837677: open the VNC console in a new window #5548

Closed

		const ConnectedTerminalToolicon = connectToFlags(FLAG_DEVWORKSPACE)(TerminalToolicon);

		export default ConnectedTerminalToolicon;

	<CloudShellDrawer open={open} onClose={onClose}>
	<CloudShellDrawer open={open} onClose={() => closeTerminalConfirmationModal(onClose)}>

Feat(cli-terminal): Create cli-terminal #4762

Feat(cli-terminal): Create cli-terminal #4762

Conversation

abhinandan13jan commented Mar 18, 2020 • edited

Feature:

Analysis / Root cause:

Solution Description:

ScreenShots

Unit tests:

Browser conformance:

How to Review:

andrewballantyne commented Mar 19, 2020

sahil143 left a comment • 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

sahil143 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

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

parvathyvr commented Mar 31, 2020

abhinandan13jan commented Mar 31, 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

abhinandan13jan commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinandan13jan commented Apr 6, 2020

sahil143 left a comment

Choose a reason for hiding this comment

abhinandan13jan commented Apr 6, 2020

sahil143 commented Apr 6, 2020

abhinandan13jan commented Apr 6, 2020

christianvogt commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 2020

openshift-bot commented Apr 6, 2020

openshift-bot commented Apr 6, 2020

openshift-bot commented Apr 6, 2020

abhinandan13jan commented Mar 18, 2020 •

edited

sahil143 left a comment •

edited

abhinandan13jan commented Mar 31, 2020 •

edited