Feat(CLI-terminal): Open terminal in new Tab #4946

abhinandan13jan · 2020-04-07T15:05:33Z

Feature:

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

The restore link has been removed after discussion with UX
This PR doesn't handle the session management for cloudshell terminal

Analysis / Root cause:

Launch Cloudshell in new Tab

Solution Description:

Creates a new Route /cloudshell to load Terminal as a full page component at this URL.
Creates a new CloudshellTab component with holds the Terminal and exposes it over the given route.

To Do

Add Unit tests ✔️
Comment out CloudShellIcon from mastheadToolbar. ✔️

ScreenShots

Unit Test

CloudShellTab.spec.tsx

Broswer conformance

Chrome 73

How to Review

Setup che-workspace-controller on cluster

clone repo https://github.com/che-incubator/che-workspace-operator
Run the following commands from repo root
export IMG=quay.io/che-incubator/che-workspace-controller:nightly
export TOOL=oc # Use 'export TOOL=kubectl' for kubernetes
make deploy

Uncomment the import and addition of CloudShellMastHeadIcon in masthead-toolbar.jsx at line 27 and 578

rohitkrai03 · 2020-04-13T11:16:46Z

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

@@ -35,8 +35,8 @@ const CloudShellDrawer: React.FC<CloudShellDrawerProps> = ({ children, onClose }
          <Button
            variant="plain"
            component="a"
+            href="/cloudshell"
            // change this once we can open terminal in new tab


Remove this comment.

rohitkrai03 · 2020-04-13T11:17:26Z

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

+import './CloudShellTab.scss';
+
+const CloudShellTabHeader = () => {
+  // const onClose = () => {


Remove comments.

rohitkrai03 · 2020-04-13T11:17:38Z

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

@@ -0,0 +1,33 @@
+import * as React from 'react';
+import { Flex, FlexItem } from '@patternfly/react-core';
+// import { FlexModifiers, Button } from '@patternfly/react-core';


Remove comments.

rohitkrai03 · 2020-04-13T11:20:38Z

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

+  //   window.close();
+  // };
+  return (
+    <Flex style={{ flexGrow: 1 }}>


Use flex modifiers prop instead of style - https://www.patternfly.org/v4/documentation/react/layouts/flex

@rohitkrai03 This portion is same as that in the CloudShellDrawerHeader

@abhinandan13jan What is the need to specify the style like this in both the cases? Wouldn't this work - breakpointMods={[{modifier: FlexModifiers.grow}]} ?

yes, it works...updated..thanks

rohitkrai03 · 2020-04-13T11:21:20Z

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

+      <FlexItem>
+        <span data-test-id="terminal-tab-header">Openshift command Line Terminal</span>
+      </FlexItem>
+      {/* <FlexItem breakpointMods={[{ modifier: FlexModifiers['align-right'] }]}>


Remove comments.

rohitkrai03 · 2020-04-13T11:22:05Z

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

+      'Openshift command Line Terminal',
+    );
+  });
+  // it('should render restore button', () => {


Remove comments.

All the commented out portions are to be brought back.. these are being temporarily hidden till we work on the session thing.

I'll update the comments with the note

Well, I would prefer no code instead of commented out code anyways. You can easily go back in git history and get the code back when needed.

Updated this

christianvogt · 2020-04-13T16:50:30Z

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

+  return (
+    <Flex breakpointMods={[{ modifier: FlexModifiers.grow }]}>
+      <FlexItem>
+        <span data-test-id="terminal-tab-header">Openshift command Line Terminal</span>


Casing isn't consistent. Also it's spelt OpenShift.

Why a flex when there is only a single piece of text?

I'll correct the Case of the text,
The Flex is there as it had a RestoreLink FlexItem to the right. Also, we'll have to add it back again.

invincibleJai · 2020-04-14T17:20:50Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss

+  width: 100%;
+  height: 100%;
+  overflow: hidden;
+  background-color: #000;


can we use pf variables for background-color as well

invincibleJai · 2020-04-14T17:21:58Z

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

+const CloudShellTab = () => {
+  return (


can we avoid explicit return statement

invincibleJai · 2020-04-14T17:27:09Z

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

+const CloudShellTabHeader = () => {
+  return (


can we avoid explicit return statement and have type React.FC

andrewballantyne · 2020-04-16T19:43:09Z

/kind feature

christianvogt · 2020-04-16T21:08:14Z

frontend/public/components/app.jsx

@@ -227,7 +227,10 @@ if ('serviceWorker' in navigator) {
 render(
  <Provider store={store}>
    <Router history={history} basename={window.SERVER_FLAGS.basePath}>
-      <Route path="/" component={App} />
+      <Switch>
+        <Route path="/cloudshell" component={CloudShellTab} />


UX doesn't want terminology of cloud shell used anywhere. Use terminal instead:

Suggested change

<Route path="/cloudshell" component={CloudShellTab} />

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

christianvogt · 2020-04-16T21:22:00Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss

@@ -0,0 +1,16 @@
+.co-cloud-shell-tab {


Suggested change

.co-cloud-shell-tab {

.co-cloud-shell-tab {

display: flex;

flex-direction: column;

christianvogt · 2020-04-16T21:22:16Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss

+  height: 100%;
+  overflow: hidden;
+  background-color: var(--pf-global--Color--dark-100);
+  &__header {


This styling should be applied to the CloudShellTabHeader instead of here.

Suggested change

&__header {

&__header {

flex-shrink: 0;

updated..Removed CloudShellheader

christianvogt · 2020-04-16T21:28:30Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss

+    background-color: var(--pf-global--palette--black-200);
+    display: flex;
+    align-items: center;
+    padding-left: var(--pf-global--spacer--md);


Suggested change

padding-left: var(--pf-global--spacer--md);

padding: 0 var(--pf-global--spacer--md);

christianvogt · 2020-04-16T21:31:11Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss

+  overflow: hidden;
+  background-color: var(--pf-global--Color--dark-100);
+  &__header {
+    height: var(--pf-global--target-size--MinHeight);


Suggested change

height: var(--pf-global--target-size--MinHeight);

min-height: var(--pf-global--target-size--MinHeight);

christianvogt · 2020-04-16T21:31:30Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.scss

+  &__restorelink {
+    color: var(--pf-global--primary-color--100);
+  }


christianvogt · 2020-04-16T21:31:54Z

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

+    <div className="co-cloud-shell-tab__header">
+      <CloudShellTabHeader />
+    </div>


I think you can simplify the header to the following and remove CloudShellTabHeader.

Suggested change

<div className="co-cloud-shell-tab__header">

<CloudShellTabHeader />

</div>

<div className="co-cloud-shell-tab__header">OpenShift command line terminal</div>

christianvogt · 2020-04-17T16:06:13Z

/lgtm
/approve

openshift-ci-robot · 2020-04-17T16:06:40Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

spadgett · 2020-04-17T21:48:40Z

/hold
for #5100

spadgett · 2020-04-17T23:16:27Z

/hold cancel
/retest

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/core Related to console core functionality labels Apr 7, 2020

openshift-ci-robot requested review from jcaianirh and spadgett April 7, 2020 15:09

abhinandan13jan force-pushed the terminal-tab branch from 5c62097 to 575dab1 Compare April 8, 2020 14:43

abhinandan13jan changed the title ~~[WIP] Feat(CLI-terminal): Open terminal in new Tab~~ Feat(CLI-terminal): Open terminal in new Tab Apr 8, 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 Apr 8, 2020

abhinandan13jan force-pushed the terminal-tab branch 2 times, most recently from c30f4f9 to 2058a46 Compare April 13, 2020 10:01

rohitkrai03 suggested changes Apr 13, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal-tab branch 3 times, most recently from a934b61 to 1b375d6 Compare April 13, 2020 14:25

christianvogt reviewed Apr 13, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal-tab branch from 1b375d6 to 472ed96 Compare April 14, 2020 08:38

invincibleJai reviewed Apr 14, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal-tab branch from 472ed96 to 8632b63 Compare April 15, 2020 12:51

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

christianvogt reviewed Apr 16, 2020

View reviewed changes

abhinandan13jan force-pushed the terminal-tab branch from 8632b63 to ff7293a Compare April 17, 2020 10:53

Feat(CLI-terminal): Open terminal in new Tab

8df6980

abhinandan13jan force-pushed the terminal-tab branch from ff7293a to 8df6980 Compare April 17, 2020 11:20

openshift-ci-robot assigned christianvogt Apr 17, 2020

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

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020

openshift-merge-robot merged commit b1729dd into openshift:master Apr 18, 2020

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

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

	padding-left: var(--pf-global--spacer--md);
	padding: 0 var(--pf-global--spacer--md);

	height: var(--pf-global--target-size--MinHeight);
	min-height: var(--pf-global--target-size--MinHeight);

Feat(CLI-terminal): Open terminal in new Tab #4946

Feat(CLI-terminal): Open terminal in new Tab #4946

Conversation

abhinandan13jan commented Apr 7, 2020 • edited

Feature:

Analysis / Root cause:

Solution Description:

ScreenShots

Unit Test

Broswer conformance

How to Review

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

rohitkrai03 Apr 13, 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

andrewballantyne commented Apr 16, 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

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

christianvogt commented Apr 17, 2020

openshift-ci-robot commented Apr 17, 2020

spadgett commented Apr 17, 2020

spadgett commented Apr 17, 2020

abhinandan13jan commented Apr 7, 2020 •

edited

rohitkrai03 Apr 13, 2020 •

edited