feat(SerialConsole): Introduce SerialConsole component #160

mareklibra · 2018-01-11T11:04:12Z

What: Provide reusable component to access serial console of a server/virtual machine

Link to Storybook: https://mareklibra.github.io/patternfly-react/

Additional issues:

Description:
The SerialConsole component wraps access to the Terminal component [1].

The component can be used to access serial console (PTY) of a server
or a virtual machine.

[1] https://github.com/xtermjs/xterm.js

mareklibra · 2018-01-12T09:12:10Z

Storybook and test will be written, so far looking for early comments.

Reuse within Cockpit works fine.
I will try in another project.

jeff-phillips-18 · 2018-01-12T12:39:28Z

src/components/SerialConsole/console.css

+        color: #f0f0f0;
+        background: #000;
+    }
+}


This should be done in less and sass and added to the appropriate directories (we will need to add the converter to this repo soon).
Colors should be from the patternfly palette.

ok, css removed.

jeff-phillips-18 · 2018-01-12T12:41:57Z

package.json

    "build:scripts": "babel src --out-dir dist/js --ignore test*.js,__mocks__",
    "build:less": "mkdir -p dist/less && cp -r less/* dist/less",
    "build:sass": "mkdir -p dist/sass && cp -r sass/patternfly-react/* dist/sass && node-sass --output-style compressed --include-path sass $npm_package_sassIncludes_patternfly $npm_package_sassIncludes_bootstrap $npm_package_sassIncludes_fontAwesome -o dist/css sass/patternfly-react.scss",
+    "build:css": "mkdir -p dist/css && find ./src -name \\*.css -exec cp {} dist/css/ \\;",


we shouldn't have css in the components directories. We've standardized on both less and sass so that we can use variables which can then be overridden.

jeff-phillips-18 · 2018-01-12T12:47:09Z

src/components/SerialConsole/XTerm.js

+  componentWillMount() {
+    const term = new Terminal({
+      cols: this.props.cols || 80,
+      rows: this.props.rows || 25,


This could be done using defaultProps

jeff-phillips-18 · 2018-01-12T12:52:24Z

src/components/SerialConsole/SerialConsole.js

+      textDisconnect = 'Disconnect',
+      textDisconnected = 'Disconnected from serial console. Click the Reconnect button.',
+      textLoading = 'Loading ...',
+      textReconnect = 'Reconnect',


These need to be override-able by applications using I18N. See https://github.com/patternfly/patternfly-react/pull/162/files#diff-9694875e1ead42ed0c55964e603992b1R6 as an example on how to do this.

That was the point here as well. Anyway, wrapped into the messages prop to be consistent.

mareklibra · 2018-01-15T12:57:17Z

less/patternfly-react.less

@@ -1,3 +1,62 @@
 /**
  Patternfly React Specific Extensions
 */
+@import '~patternfly/dist/less/variables';
+
+/** Following is used by SerialConsole */


What about introducing .less files local to the component? Means like src/SerialConsole/styles.less .
They would be listed in less/patternfly-react.less for import.

I'd be ok with that if that's what we all agree on. Still need the sass version too though.

Could you use a separate file here as well? Add this to serialConsole.less and import the file here.

mareklibra · 2018-01-17T08:12:36Z

Sass added

jeff-phillips-18 · 2018-01-17T12:14:16Z

sass/patternfly-react.scss

@@ -21,3 +21,61 @@ $icon-font-path: '~patternfly/dist/fonts/';
 */

 @import 'patternfly-react/patternfly-react';
+
+/** Following is used by SerialConsole */


This should go in sass/patternfly-react/_serialConsole.sass and then add @import 'serialConsole' to sass/patternfly-react/_patternfly-react.scss

mareklibra · 2018-01-17T14:35:59Z

less/serialConsole.less added

jeff-phillips-18 · 2018-01-18T14:39:18Z

Looking at this more closely, I'm not really sure it fits in this repository. There is no Patternfly pattern for this and no existing Patternfly styling for this.

Might this be better as its own standalone component? react-serial-console for instance?

ohadlevy · 2018-01-18T15:06:47Z

@jeff-phillips-18 - we have discussed that components that are common to many patternfly users - we would like to keep under the patternfly organization, since this is the first one, we also agreed that initially we would keep it in this repo, and over time, extract to separate npm packages, so that users can upgrade as needed. /cc @serenamarie125

priley86 · 2018-01-19T21:19:55Z

src/components/SerialConsole/SerialConsole.js

+  constructor(props) {
+    super(props);
+
+    this.focusTerminal = this.focusTerminal.bind(this);


can we use:
import { bindMethods } from '../../common/helpers';

instead?

done, nice helper function :-)

priley86 · 2018-01-19T21:20:46Z

src/components/SerialConsole/SerialConsole.js

+    this.props.onDisconnect();
+  }
+
+  msg(key) {


I don't really get it... why not just use defaultProps and specify the value? Maybe someone can explain this...

Based on a previous review comment, I expect this is a pattern in this project. If not, the former direct use of props seems to me more straightforward.

done, defaultProps used instead

priley86 · 2018-01-19T21:22:53Z

src/components/SerialConsole/SerialConsole.js

+    let terminal;
+    let disconnectDisabled = 'disabled'; // CSS class name
+    switch (status) {
+      case 'connected':


maybe break out the statuses into a constants.js or SerialConsoleConstants.js?

jeff-phillips-18 · 2018-01-22T22:03:25Z

less/serialConsole.less

+  SerialConsole styles
+ */
+
+.console-ct > .terminal {


What is the -ct? It should likely be -pf

It's a leftover from original code. The .console-ct is used for the top-most div of the component, like a container.
The console-pf in this case sounds good to me as well.

jeff-phillips-18 · 2018-01-22T22:05:17Z

less/serialConsole.less

+  line-height: normal;
+}
+
+@media (min-width: 568px) {


Would it still work if we used one of the bootstrap variables @screen-sm or @screen-xs or is there a reason for using 568?

@screen-tablet used instead due to deprecation of the others. Just verified, still works well.

jeff-phillips-18 · 2018-01-22T22:08:35Z

less/serialConsole.less

+
+@media (min-width: 568px) {
+  .console-ct {
+    font-size: 12px;


@font-size-small

serenamarie125 · 2018-01-24T00:15:29Z

@mareklibra from a design pattern perspective, what does this feature consist of ? Are the Disconnect/Reconnect buttons a part of the pattern? Since we don't have an existing PatternFly design, I'd like to make sure that I understand the use case and design of the pattern itself. Thanks!

mareklibra · 2018-01-24T12:13:55Z

@serenamarie125 , the component is planed to be part of a higher level one - serving all consoles. For better understanding, please have a look at screenshots in later comments of cockpit-project/cockpit#8234 .

Regarding the Serial Console, it consists of

terminal (receiving input from keyboard and rendering output produced by server's serial console)
control buttons Disconnect, Reconnect

I would keep the control buttons in the pattern, they server common use cases:

disconnect from the console to free it for anyone else
reconnect to either take it back or re-establish the connection

sharvit · 2018-01-28T13:04:44Z

Thanks @mareklibra Looks good!

Wondering...

shouldn't the css part of it need to live under patternfly core?
should it live in a separate npm package (under patternfly namespace) since most of patternfly-react consumers are not going to use it? (It adds another dependency)

martinpitt · 2018-01-29T16:27:59Z

I just want to reiterate that the cockpit fork of term.js is not meant to be proliferated like that. As soon as xterm.js becomes suitable for Cockpit, we are going to move to that. So putting it as-is into PatternFly seems a bit ambitious to me.

@mareklibra : For your purposes of sharing the component between software, wouldn't a new NPM project be more suitable?

mareklibra · 2018-04-16T10:15:39Z

Rebased.
xterm version increased to 3.3.0.

mareklibra · 2018-04-16T10:15:47Z

Since implementation of the "monorepo" will still take some time plus additional will be needed to settle-down, can this PR be unblocked on it?

The xterm dependency is peerDependency only.
Repeated rebasing can introduce dummy bugs and costs additional effort.
The functionality is blocked in related projects.

mareklibra · 2018-04-16T10:19:56Z

Approvals are received prior a couple of last rebases.

serenamarie125 · 2018-04-23T13:57:46Z

@jgiardino this still has the cssreview tag on it. Can you take a look please?

jeff-phillips-18 · 2018-04-23T14:04:14Z

@mareklibra with the merge of #311, we've updated where the consoles code should live. Can you please update your code to be under https://github.com/patternfly/patternfly-react/tree/master/packages/console ?

mareklibra · 2018-04-25T14:45:23Z

Rebased and basically verified in Cockpit.

Some of the original code was already copied from here to #311 .

jeff-phillips-18 · 2018-04-25T16:30:01Z

less/patternfly-react.less

@@ -1,6 +1,9 @@
 /**
  Patternfly React Specific Extensions
 */
+@import "~bootstrap/less/variables";
+@import "~patternfly/dist/less/variables";
+


We don't want to do this here. Patternfly is a dependency and we assume that these are already included if the project is building with the less files. If we do this here, it will overwrite any variables the application might have already overridden (which would likely be the reason to include the less files rather than the CSS).

jeff-phillips-18 · 2018-04-25T16:30:14Z

packages/console/less/console.less

@@ -1,6 +1,9 @@
 /**
 +  Styling shared by both VncConsole and SerialConsole.
 +*/
+@import "~bootstrap/less/variables";
+@import "~patternfly/dist/less/variables";


Same comment as above.

jeff-phillips-18 · 2018-04-26T12:28:29Z

Please add the sass counterpart to the less styling.

mareklibra · 2018-04-26T13:59:38Z

@jeff-phillips-18 , sass was one of the parts which was copied from this PR to #311 and committed there, track seems to be lost by this step. But should be up-to-date. Please correct me.

Or do you suggest to extend the packages/console/README.md ? I can extend it, but the way sass is handled here seems to be self-enclosing to me - dependencies (like variables from bootstrap or patternfly) are imported by patternfly-react's .scss. So this looks like consistent across whole patternfly-react.

jeff-phillips-18 · 2018-04-26T14:35:37Z

Ok, sorry @mareklibra , I didn't realize it was included with content in #311

jeff-phillips-18 · 2018-04-26T14:37:42Z

packages/console/src/SerialConsole/SerialConsole.js

+  constructor(props) {
+    super(props);
+
+    bindMethods(this, ['focusTerminal', 'onResetClick', 'onDisconnectClick']);


We have deprecated bindMethods in favor of class properties.

The SerialConsole component wraps the Terminal component [1] to access serial console (PTY) of a server or a virtual machine. Part of the code (like styling) was already committed within #4a69ebd806. [1] https://github.com/xtermjs/xterm.js

jeff-phillips-18

LGTM, thanks @mareklibra !

mareklibra · 2018-05-02T08:00:00Z

Thanks.
Is there anything missing to have this merged, please?

serenamarie125

Thanks @mareklibra trying to get another maintainer to review today

priley86 · 2018-05-02T12:41:50Z

this looks great! thanks @mareklibra

can #288 be closed now? or are you planning to revisit Vnc console after?

mareklibra · 2018-05-02T12:47:12Z

can #288 be closed now?

No, it's follow-up for this adding VncConsole component.
Once this is merged, I'll rebase there.

FYI., on top of the VncConsole PR, there will be others - for

external clients
switching among console types
console configuration
probably RDP
maybe SPICE

mareklibra · 2018-05-02T13:16:55Z

Nice to see this merged :-)
What is the process for publishing @patternfly-react/console, please?

mareklibra force-pushed the serialConsole branch from 7404aaf to 6c3c50e Compare January 11, 2018 11:08

mareklibra mentioned this pull request Jan 11, 2018

machines: Refactor SerialConsole for its transition to patternfly-react cockpit-project/cockpit#8402

Closed

2 tasks

mareklibra force-pushed the serialConsole branch 2 times, most recently from eac0cb4 to f6985a3 Compare January 11, 2018 12:09

jeff-phillips-18 reviewed Jan 12, 2018

View reviewed changes

mareklibra force-pushed the serialConsole branch from f6985a3 to cef55cf Compare January 15, 2018 12:54

mareklibra commented Jan 15, 2018

View reviewed changes

mareklibra force-pushed the serialConsole branch 2 times, most recently from b62feef to 4f28d65 Compare January 17, 2018 08:10

mareklibra force-pushed the serialConsole branch from 4f28d65 to 3e30762 Compare January 17, 2018 10:48

jeff-phillips-18 reviewed Jan 17, 2018

View reviewed changes

mareklibra force-pushed the serialConsole branch 2 times, most recently from 06a80f6 to f8177b7 Compare January 17, 2018 14:34

mareklibra force-pushed the serialConsole branch 3 times, most recently from 61e1b17 to 46c7821 Compare January 18, 2018 12:44

priley86 reviewed Jan 19, 2018

View reviewed changes

jeff-phillips-18 reviewed Jan 22, 2018

View reviewed changes

mareklibra force-pushed the serialConsole branch from 46c7821 to 873b3fd Compare January 24, 2018 11:55

mareklibra force-pushed the serialConsole branch from 8030b56 to e9597c8 Compare April 25, 2018 14:43

mareklibra force-pushed the serialConsole branch 2 times, most recently from 2b975e1 to 4a08e43 Compare April 25, 2018 15:06

jeff-phillips-18 reviewed Apr 25, 2018

View reviewed changes

mareklibra force-pushed the serialConsole branch from 4a08e43 to 553ba2d Compare April 26, 2018 07:17

jeff-phillips-18 requested changes Apr 26, 2018

View reviewed changes

mareklibra added 2 commits April 27, 2018 10:15

fix(travis): run yarn install in packages/console

a4882c6

mareklibra force-pushed the serialConsole branch from 553ba2d to a4882c6 Compare April 27, 2018 08:15

jeff-phillips-18 added css approved and removed css review labels Apr 27, 2018

jeff-phillips-18 approved these changes Apr 27, 2018

View reviewed changes

serenamarie125 approved these changes May 2, 2018

View reviewed changes

priley86 approved these changes May 2, 2018

View reviewed changes

jeff-phillips-18 merged commit 466a5d8 into patternfly:master May 2, 2018

karelhala mentioned this pull request Jun 13, 2018

[WIP] feat(Tagging) Introduce Tagging React component #351

Closed

feat(SerialConsole): Introduce SerialConsole component #160

feat(SerialConsole): Introduce SerialConsole component #160

Conversation

mareklibra commented Jan 11, 2018 • edited Loading

mareklibra commented Jan 12, 2018

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

mareklibra commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra commented Jan 17, 2018

jeff-phillips-18 commented Jan 18, 2018

ohadlevy commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

mareklibra Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 Jan 19, 2018 • edited Loading

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

serenamarie125 commented Jan 24, 2018

mareklibra commented Jan 24, 2018

sharvit commented Jan 28, 2018

martinpitt commented Jan 29, 2018

mareklibra commented Apr 16, 2018

mareklibra commented Apr 16, 2018

mareklibra commented Apr 16, 2018

serenamarie125 commented Apr 23, 2018

jeff-phillips-18 commented Apr 23, 2018

mareklibra commented Apr 25, 2018 • edited Loading

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

jeff-phillips-18 commented Apr 26, 2018

mareklibra commented Apr 26, 2018 • edited Loading

jeff-phillips-18 commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

mareklibra commented May 2, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

priley86 commented May 2, 2018 • edited Loading

mareklibra commented May 2, 2018

mareklibra commented May 2, 2018

mareklibra commented Jan 11, 2018 •

edited

Loading

priley86 Jan 19, 2018 •

edited

Loading

mareklibra Jan 20, 2018 •

edited

Loading

priley86 Jan 19, 2018 •

edited

Loading

mareklibra commented Apr 25, 2018 •

edited

Loading

mareklibra commented Apr 26, 2018 •

edited

Loading

priley86 commented May 2, 2018 •

edited

Loading