Fixes based on review comments (#4)

abhinandan13jan · May 22, 2020 · 34b8402 · 34b8402
1 parent b6de504
commit 34b8402
Show file tree

Hide file tree

Showing 7 changed files with 60 additions and 59 deletions.
diff --git a/frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss b/frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss
@@ -1,6 +1,5 @@
 .co-cloud-shell-tab {
   &__body {
-    background-color: #000;
     display: flex;
     flex: 1;
     flex-direction: column;

diff --git a/frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.scss b/frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.scss
@@ -1,8 +1,7 @@
-.odc-cloudshell-terminal {
+.co-cloudshell-terminal {
   &__container {
     background-color: #000;
     color: var(--pf-global--Color--light-100);
-    width: 100%;
     height: 100%;
   }
 }
diff --git a/frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.tsx b/frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.tsx
@@ -3,7 +3,7 @@ import { connect } from 'react-redux';
 import { RootState } from '@console/internal/redux';
 import { referenceForModel } from '@console/internal/module/k8s/k8s';
 import { useK8sWatchResource } from '@console/internal/components/utils/k8s-watch-hook';
-import { StatusBox } from '@console/internal/components/utils/status-box';
+import { StatusBox, LoadError } from '@console/internal/components/utils/status-box';
 import { WorkspaceModel } from '../../models';
 import CloudshellExec from './CloudShellExec';
 import TerminalLoadingBox from './TerminalLoadingBox';
@@ -38,7 +38,6 @@ const resource = {
 const CloudShellTerminal: React.FC<CloudShellTerminalProps> = ({ username, onCancel }) => {
   const [data, loaded, loadError] = useK8sWatchResource<CloudShellResource[]>(resource);
   const [initData, setInitData] = React.useState<TerminalInitData>();
-  const [initDataLoading, setInitDataLoading] = React.useState<boolean>(false);
   const [initError, setInitError] = React.useState<string>();
   let workspace: CloudShellResource;
   let workspaceName: string;
@@ -58,17 +57,12 @@ const CloudShellTerminal: React.FC<CloudShellTerminalProps> = ({ username, onCan
     let unmounted = false;
 
     if (workspacePhase === 'Running') {
-      setInitDataLoading(true);
       initTerminal(username, workspaceName, workspaceNamespace)
         .then((res: TerminalInitData) => {
-          if (unmounted) return;
-          setInitData(res);
-          setInitDataLoading(false);
+          if (!unmounted) setInitData(res);
         })
         .catch(() => {
-          if (unmounted) return;
-          setInitDataLoading(false);
-          setInitError('Failed to connect to your OpenShift command line terminal');
+          if (!unmounted) setInitError('Failed to connect to your OpenShift command line terminal');
         });
     }
 
@@ -77,27 +71,27 @@ const CloudShellTerminal: React.FC<CloudShellTerminalProps> = ({ username, onCan
     };
   }, [username, workspaceName, workspaceNamespace, workspacePhase]);
 
-  if (loadError || initError) {
+  if (loadError) {
     return (
-      <StatusBox
-        loaded={loaded}
-        loadError={loadError || initError}
-        label="OpenShift command line terminal"
-      />
+      <StatusBox loaded={loaded} loadError={loadError} label="OpenShift command line terminal" />
     );
   }
 
-  if (!loaded || initDataLoading) {
+  if (initError) {
+    return <LoadError message={initError} label="OpenShift command line terminal" />;
+  }
+
+  if (!loaded || (workspaceName && !initData)) {
     return (
-      <div className="odc-cloudshell-terminal__container">
+      <div className="co-cloudshell-terminal__container">
         <TerminalLoadingBox />
       </div>
     );
   }
 
   if (initData && workspaceNamespace) {
     return (
-      <div className="odc-cloudshell-terminal__container">
+      <div className="co-cloudshell-terminal__container">
         <CloudshellExec
           namespace={workspaceNamespace}
           container={initData.container}
@@ -108,9 +102,7 @@ const CloudShellTerminal: React.FC<CloudShellTerminalProps> = ({ username, onCan
     );
   }
 
-  if (loaded && !workspace) return <CloudShellSetup onCancel={onCancel} />;
-
-  return null;
+  return <CloudShellSetup onCancel={onCancel} />;
 };
 
 // For testing

diff --git a/frontend/packages/console-app/src/components/cloud-shell/Terminal.scss b/frontend/packages/console-app/src/components/cloud-shell/Terminal.scss
@@ -0,0 +1,8 @@
+.co-terminal {
+  height: 100%;
+
+  > .terminal {
+    height: 100%;
+  }
+}
+
diff --git a/frontend/packages/console-app/src/components/cloud-shell/Terminal.tsx b/frontend/packages/console-app/src/components/cloud-shell/Terminal.tsx
@@ -1,14 +1,15 @@
 import * as React from 'react';
-import { Terminal as XTerminal } from 'xterm';
+import { Terminal as XTerminal, ITerminalOptions } from 'xterm';
 import { fit } from 'xterm/lib/addons/fit/fit';
 
-const terminalOptions = {
+import './Terminal.scss';
+
+const terminalOptions: ITerminalOptions = {
   fontFamily: 'monospace',
   fontSize: 16,
   cursorBlink: false,
   cols: 80,
   rows: 25,
-  padding: 4,
 };
 
 type TerminalProps = {
@@ -26,30 +27,8 @@ const Terminal = React.forwardRef<ImperativeTerminalType, TerminalProps>(({ onDa
   const terminal = React.useRef<XTerminal>();
   const terminalRef = React.useRef<HTMLDivElement>();
 
-  const focus = () => {
-    terminal.current && terminal.current.focus();
-  };
-
-  const reset = () => {
-    if (!terminal.current) return;
-    terminal.current.reset();
-    terminal.current.clear();
-    terminal.current.setOption('disableStdin', false);
-  };
-
-  const onDataReceived = (data) => {
-    terminal.current && terminal.current.write(data);
-  };
-
-  const onConnectionClosed = (msg) => {
-    if (!terminal.current) return;
-    terminal.current.write(`\x1b[31m${msg || 'disconnected'}\x1b[m\r\n`);
-    terminal.current.setOption('disableStdin', true);
-  };
-
   React.useEffect(() => {
     const term: XTerminal = new XTerminal(terminalOptions);
-    term.on('data', onData);
     term.open(terminalRef.current);
     term.focus();
 
@@ -68,16 +47,38 @@ const Terminal = React.forwardRef<ImperativeTerminalType, TerminalProps>(({ onDa
       term.destroy();
       resizeObserver.disconnect();
     };
+  }, []);
+
+  React.useEffect(() => {
+    const term = terminal.current;
+    term.on('data', onData);
+
+    return () => {
+      term.off('data', onData);
+    };
   }, [onData]);
 
   React.useImperativeHandle(ref, () => ({
-    focus,
-    reset,
-    onDataReceived,
-    onConnectionClosed,
+    focus: () => {
+      terminal.current && terminal.current.focus();
+    },
+    reset: () => {
+      if (!terminal.current) return;
+      terminal.current.reset();
+      terminal.current.clear();
+      terminal.current.setOption('disableStdin', false);
+    },
+    onDataReceived: (data) => {
+      terminal.current && terminal.current.write(data);
+    },
+    onConnectionClosed: (msg) => {
+      if (!terminal.current) return;
+      terminal.current.write(`\x1b[31m${msg || 'disconnected'}\x1b[m\r\n`);
+      terminal.current.setOption('disableStdin', true);
+    },
   }));
 
-  return <div ref={terminalRef} style={{ width: '100%', height: '100%' }} />;
+  return <div className="co-terminal" ref={terminalRef} />;
 });
 
 export default Terminal;
diff --git a/frontend/packages/console-app/src/components/cloud-shell/TerminalLoadingBox.tsx b/frontend/packages/console-app/src/components/cloud-shell/TerminalLoadingBox.tsx
@@ -4,6 +4,7 @@ import { LoadingBox } from '@console/internal/components/utils/status-box';
 type TerminalLoadingBoxProps = {
   message?: string;
 };
+
 const TerminalLoadingBox: React.FC<TerminalLoadingBoxProps> = ({ message }) => (
   <LoadingBox message={message || 'Connecting to your OpenShift command line terminal ...'} />
 );

diff --git a/...end/packages/console-app/src/components/cloud-shell/__tests__/TerminalLoadingBox.spec.tsx b/...end/packages/console-app/src/components/cloud-shell/__tests__/TerminalLoadingBox.spec.tsx
@@ -1,19 +1,20 @@
 import * as React from 'react';
-import { shallow, ShallowWrapper } from 'enzyme';
+import { shallow } from 'enzyme';
 import { LoadingBox } from '@console/internal/components/utils/status-box';
 import TerminalLoadingBox from '../TerminalLoadingBox';
 
 describe('TerminalLoadingBox', () => {
-  it('should render cloudshellterminal', () => {
-    const terminalLoadingWrapper: ShallowWrapper = shallow(<TerminalLoadingBox />);
+  it('should send default message if message prop is not there', () => {
+    const terminalLoadingWrapper = shallow(<TerminalLoadingBox />);
     const loadingBox = terminalLoadingWrapper.find(LoadingBox);
     expect(loadingBox.exists()).toBe(true);
     expect(loadingBox.prop('message')).toEqual(
       'Connecting to your OpenShift command line terminal ...',
     );
   });
-  it('should render cloudshellterminal', () => {
-    const terminalLoadingWrapper: ShallowWrapper = shallow(<TerminalLoadingBox message="test" />);
+
+  it('should forward message prop to loading box component', () => {
+    const terminalLoadingWrapper = shallow(<TerminalLoadingBox message="test" />);
     const loadingBox = terminalLoadingWrapper.find(LoadingBox);
     expect(loadingBox.exists()).toBe(true);
     expect(loadingBox.prop('message')).toEqual('test');