feat(VncConsole): Introduce VncConsole component #288

mareklibra · 2018-03-26T11:20:47Z

What:
Provide reusable component to access server/virtual machine via VNC protocol.

Link to Storybook:
Mock VNC backend is not available, so storybook is just a formal one, not showing the functionality.
Screenshots will substitute it in this case.

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

Additional issues:
Depends on: ~~#160 (SerialConsole)~~

Future work:
If this change is accepted, higher-level <Consoles/> component wrapping selection of either VncConsole or SerialConsole will be implemented.

coveralls · 2018-03-26T13:12:54Z

Pull Request Test Coverage Report for Build 1055

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
9 unchanged lines in 2 files lost coverage.
Overall coverage increased (+8.9%) to 80.609%

Files with Coverage Reduction	New Missed Lines	%
src/common/patternfly.js	1	71.43%
src/common/helpers.js	8	39.39%

Totals
Change from base Build 1043:	8.9%
Covered Lines:	852
Relevant Lines:	995

💛 - Coveralls

coveralls · 2018-03-26T13:12:54Z

Pull Request Test Coverage Report for Build 1364

26 of 48 (54.17%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-18.2%) to 55.885%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/console/src/VncConsole/VncActions.js	3	4	75.0%
packages/console/src/VncConsole/VncConsole.js	21	42	50.0%

Totals
Change from base Build 1359:	-18.2%
Covered Lines:	5744
Relevant Lines:	8678

💛 - Coveralls

mareklibra · 2018-03-26T13:13:03Z

First use of the component will be in the cockpit-project.org.

mareklibra · 2018-03-26T13:19:11Z

Part of the pattern is the element with VNC session and action button sending Ctrl+Alt+Del to the VNC server.
Please note, the combo-box in the left upper corner is not part of the pattern - it will be included in a higher-level component later.

The component resizes according to backend:

mareklibra · 2018-03-26T13:29:14Z

The npm test recently fails on ES6 code under node_modules.
Any suggestion how to fix that?

Tests for unrelated components (like Toolbar) are failing since they import other patternfly-react components via src/index.js which transitively imports src/VncConsole/index.js and so the failing node_modules/@novnc/novnc/core/*.js .

Selected node_modules subdirectories need to be babelized for jest.

There is no such issue for npm run build, just for the tests.

For consuming application, it can be fixed i.e. like within cockpit-project/cockpit#8894 (see change to webpack.config.js).

mareklibra · 2018-03-27T13:30:19Z

The issue is fixed similar way as for Cockpit - by adding transformIgnorePatterns to jest configuration

mareklibra · 2018-03-27T13:57:43Z

Remains issue with noVNC in missing 'MutationObserver' in novnc/core/util/events.js.
Will fix in the noVNC project.

mareklibra · 2018-03-28T07:24:37Z

Workaround for missing MutationObserver class within jest testing is added.
Keep trying to fix it in noVNC.

mareklibra · 2018-03-29T08:10:23Z

Added babel-loader for node_modules/@novnc required by storybook build

mareklibra · 2018-04-16T13:01:13Z

Rebased

jeff-phillips-18 · 2018-04-23T14:05:09Z

@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-05-03T10:16:49Z

Rebased since #160 is merged.

mareklibra · 2018-05-03T10:17:21Z

This is ready for review.

mareklibra · 2018-05-10T09:45:53Z

Rebased after #332

mareklibra · 2018-05-10T09:46:16Z

May I ask for review, please?

jeff-phillips-18 · 2018-05-10T10:32:55Z

packages/console/src/VncConsole/VncActions.js

+    {textSendShortcut}
+
+    <Button
+      bsClass="btn btn-default console-actions-buttons-pf"


no need for btn or btn-default

michalskrivanek · 2018-06-18T12:08:51Z

@michalskrivanek , ideas?
I.e. Alt-F4 ? Ctrl-W ?

take a look at virt-viewer;-)

mareklibra · 2018-06-18T13:14:23Z

I.e. Alt-F4 ? Ctrl-W ?

take a look at virt-viewer;-)

So what to include in the pattern? All the shurtcuts implemented within virt-viewer?
It would mean: Ctrl+Alt+Backspace , Ctrl+Alt+Del , Ctrl+Alt+F1 .. F12 , Printscreen

In addition, Alt-F4 is handed by browser recently, causing it's window to be closed.

Or provide an extension mechanism to add additional ones?

serenamarie125 · 2018-06-19T00:46:31Z

@mareklibra might it be possible to allow for the following by default Ctrl+Alt+Backspace , Ctrl+Alt+Del & Printscreen (but allow them to be not shown)? You could also allow for a mechanism to add additional commands, but that could be a future contribution as well.

serenamarie125 · 2018-06-19T00:46:57Z

packages/console/src/VncConsole/VncActions.js

+  onCtrlAltDel: noop,
+
+  textCtrlAltDel: 'Ctrl+Alt+Del',
+  textSendShortcut: 'Send key'


please use initial caps ... "Send Key"

serenamarie125 · 2018-06-19T00:47:23Z

packages/console/src/VncConsole/__snapshots__/VncActions.test.js.snap

+  bsStyle="default"
+  id="console-send-shortcut"
+  onClick={[Function]}
+  title="Send key"


please use initial caps ... "Send Key"

mareklibra · 2018-06-22T06:46:15Z

@serenamarie125 , this makes sense.
I can add mechanism to append custom combinations with handful predefined ones.

If it is ok, I would postpone that work to a follow-up to avoid further prolonging merge of this.

mareklibra · 2018-06-22T07:15:08Z

Rebased.
Build is recently failing on unrelated issues

ApplicationLauncherWrapper
styling

serenamarie125 · 2018-06-22T14:21:55Z

If it is ok, I would postpone that work to a follow-up to avoid further prolonging merge of this.

+1 sounds good, thank you! I've created #422 to track the enhancement.

serenamarie125

Thanks for the contribution!

dmiller9911 · 2018-03-31T14:10:42Z

src/test.env.js

+
+// referenced from '@novnc/nvnc/core/util/events.js'
+// The MutationObserver is available in supported browsers, this is workaround for "jest"
+global.MutationObserver = global.MutationObserver || MutationObserverPolyfill;


This seems like the right way to do this. I am not sure if the noVNC lib will be able to fix this since it is a test environment specific thing. I am good with leaving this here if others are.

dmiller9911 · 2018-06-22T14:59:25Z

package.json

@@ -167,6 +168,9 @@
    "roots": [
      "<rootDir>/packages",
      "<rootDir>/scripts"
+    ],
+    "transformIgnorePatterns": [
+      "node_modules/(?!@novnc)"


this needs to be added to the one on line 165. Looks like a rebase issue.

"transformIgnorePatterns": [ "node_modules/(?!@patternfly)", "node_modules/(?!@novnc)" ],

That should fix the build

right, a rebase issue.
Thanks for spotting this.

jeff-phillips-18

LGTM once the build is working again.

beanh66

LGTM, thanks @mareklibra!

mareklibra · 2018-06-26T11:08:11Z

Rebased and fixed the former rebase issue with package.json

affects: @patternfly/react-console New VncComponent is introduced for patternfly/react-console. The component wraps HTML-based noVNC client [1] [1] https://github.com/novnc/noVNC

…environment affects: @patternfly/react-console, patternfly-react Referenced from noVNC (@novnc/nvnc/core/util/events.js). Workaround for the 'jest' testing only since the MutationObserver is available in browsers otherwise. Attempt to fix the issue by using 'browser' jest configuration option did not work in this case.

mareklibra · 2018-06-29T06:34:10Z

May I ask for (final) review? @dmiller9911 , @jeff-phillips-18

jeff-phillips-18 · 2018-07-02T17:31:44Z

Going to merge this though we dropped coverage. Hopefully we can address this soon.

mareklibra force-pushed the vncConsole branch from 318417a to 229af92 Compare March 26, 2018 11:25

mareklibra changed the title ~~feat(VncConsole): Introduce VncConsole component~~ WIP: feat(VncConsole): Introduce VncConsole component Mar 26, 2018

mareklibra mentioned this pull request Mar 26, 2018

Use noVNC via patternfly-react cockpit-project/cockpit#8894

Merged

4 tasks

mareklibra force-pushed the vncConsole branch from 229af92 to 4b2a4fb Compare March 26, 2018 13:09

mareklibra force-pushed the vncConsole branch from 4b2a4fb to 8b5f08b Compare March 27, 2018 13:29

mareklibra force-pushed the vncConsole branch from 2db7669 to da66dbc Compare March 29, 2018 08:06

mareklibra force-pushed the vncConsole branch 2 times, most recently from 887bb9a to 0fd32af Compare April 16, 2018 12:47

priley86 mentioned this pull request Apr 18, 2018

chore(Console): Add skeleton of @patternfly-react/console package #311

Merged

mareklibra mentioned this pull request Apr 25, 2018

machines: Add create vm dialog cockpit-project/cockpit#7820

Merged

17 tasks

priley86 mentioned this pull request May 2, 2018

feat(SerialConsole): Introduce SerialConsole component #160

Merged

mareklibra force-pushed the vncConsole branch from 0fd32af to e6b2ca4 Compare May 3, 2018 10:15

mareklibra changed the title ~~WIP: feat(VncConsole): Introduce VncConsole component~~ feat(VncConsole): Introduce VncConsole component May 3, 2018

mareklibra force-pushed the vncConsole branch from e6b2ca4 to fdc424e Compare May 10, 2018 09:45

jeff-phillips-18 reviewed May 10, 2018

View reviewed changes

serenamarie125 requested changes Jun 19, 2018

View reviewed changes

bond95 mentioned this pull request Jun 19, 2018

[RFE] Add ability to use NoVNC console in VM Portal oVirt/ovirt-web-ui#640

Closed

mareklibra force-pushed the vncConsole branch from 179318b to f302101 Compare June 22, 2018 07:08

serenamarie125 mentioned this pull request Jun 22, 2018

VncConsole - Send Key menu enhancements #422

Closed

3 tasks

serenamarie125 previously approved these changes Jun 22, 2018

View reviewed changes

serenamarie125 added ux approved and removed ux review labels Jun 22, 2018

dmiller9911 suggested changes Jun 22, 2018

View reviewed changes

jeff-phillips-18 previously approved these changes Jun 22, 2018

View reviewed changes

beanh66 previously approved these changes Jun 25, 2018

View reviewed changes

mareklibra dismissed stale reviews from beanh66, jeff-phillips-18, and serenamarie125 via fe804bf June 26, 2018 11:07

mareklibra force-pushed the vncConsole branch from f302101 to fe804bf Compare June 26, 2018 11:07

mareklibra added 2 commits June 26, 2018 13:45

feat(VncConsole): introduce VncConsole component

4199927

affects: @patternfly/react-console New VncComponent is introduced for patternfly/react-console. The component wraps HTML-based noVNC client [1] [1] https://github.com/novnc/noVNC

mareklibra force-pushed the vncConsole branch from fe804bf to 6e0d6f0 Compare June 26, 2018 11:45

jeff-phillips-18 approved these changes Jun 29, 2018

View reviewed changes

dmiller9911 approved these changes Jul 2, 2018

View reviewed changes

jeff-phillips-18 merged commit 6699427 into patternfly:master Jul 2, 2018

gregsheremeta mentioned this pull request Aug 11, 2018

Support web-based console connection oVirt/ovirt-web-ui#490

Closed

feat(VncConsole): Introduce VncConsole component #288

feat(VncConsole): Introduce VncConsole component #288

Conversation

mareklibra commented Mar 26, 2018 • edited

coveralls commented Mar 26, 2018

Pull Request Test Coverage Report for Build 1055

💛 - Coveralls

coveralls commented Mar 26, 2018 • edited

Pull Request Test Coverage Report for Build 1364

💛 - Coveralls

mareklibra commented Mar 26, 2018

mareklibra commented Mar 26, 2018 • edited

mareklibra commented Mar 26, 2018

mareklibra commented Mar 27, 2018

mareklibra commented Mar 27, 2018

mareklibra commented Mar 28, 2018

mareklibra commented Mar 29, 2018

mareklibra commented Apr 16, 2018

jeff-phillips-18 commented Apr 23, 2018

mareklibra commented May 3, 2018

mareklibra commented May 3, 2018

mareklibra commented May 10, 2018

mareklibra commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalskrivanek commented Jun 18, 2018

mareklibra commented Jun 18, 2018

serenamarie125 commented Jun 19, 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

mareklibra commented Jun 22, 2018 • edited

mareklibra commented Jun 22, 2018

serenamarie125 commented Jun 22, 2018

serenamarie125 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

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

beanh66 left a comment

Choose a reason for hiding this comment

mareklibra commented Jun 26, 2018

mareklibra commented Jun 29, 2018

jeff-phillips-18 commented Jul 2, 2018

mareklibra commented Mar 26, 2018 •

edited

coveralls commented Mar 26, 2018 •

edited

mareklibra commented Mar 26, 2018 •

edited

mareklibra commented Jun 22, 2018 •

edited