feat(VncConsole): More possibilities to customize component #1402

bond95 · 2019-02-18T14:49:04Z

What:

Added possibility to extract toolbar to any other component. Added scaling prop to VncConsole, and
enhance textConnecting props to use node type as well. And fixed disconnecting.

We need this features for console page in oVirt VM Portal: oVirt/ovirt-web-ui#956

Example from oVirt VM Portal:

@mareklibra please, review

mareklibra · 2019-02-19T13:03:48Z

packages/patternfly-3/react-console/src/VncConsole/VncConsole.js

    this.disconnect();
+    this.removeEventListeners();
+    this.rfb = undefined;


With this change, the RFB.disconnect() is skipped. Can you please explain reason for that?

But no, it doesn't skipped. Line 66 perform disconnecting. And previously onDisconnected event doesn't work, because it removed all listeners before it disconnected.

mareklibra · 2019-02-19T13:03:57Z

packages/patternfly-3/react-console/src/VncConsole/VncConsole.js

  }

-  disconnect() {
+  disconnect = () => {


Why this change, please?

Because otherwise method doesn't get this context.

mareklibra · 2019-02-19T13:05:16Z

packages/patternfly-3/react-console/src/VncConsole/VncActions.js

+  textDisconnect,
+  onCtrlAltDel,
+  onDisconnect,
+  portalToolbarTo


The name is too specific for the oVirt VM Portal

Well, it calling portalToolbarTo, because if it set it do portal toolbar to some element, not because this is from VM Portal. But yeah, it better to rename.

mareklibra · 2019-02-19T13:07:33Z

packages/patternfly-3/react-console/src/VncConsole/VncActions.js

+    return toolbar;
+  }
+  return (
+    document.getElementById(portalToolbarTo) && ReactDOM.createPortal(toolbar, document.getElementById(portalToolbarTo))


I don't think we should perform such manipulations. I believe change in the component composition should be preferred over this approach.

Yes, but in that case appear problem with customization. For example, in VM Portal we need only one toolbar not two and we can't Send Key dropdown move to our toolbar, because console is fully under control of VncConsole component.

And I think this is most suitable solution.

mareklibra · 2019-02-19T13:09:50Z

packages/patternfly-3/react-console/src/VncConsole/VncActions.js

 };

 VncActions.defaultProps = {
  onCtrlAltDel: noop,
+  onDisconnect: noop,


This is a breaking change.
The consuming application is silently required to add implementation. Otherwise the button just appears there without any function.

Ok, I'll do onDisconnect props required, but this is was just copy from onCtlAltDel. So why in this case onCtrlAltDel: noop not required too?

mareklibra · 2019-02-19T13:10:05Z

packages/patternfly-3/react-console/src/VncConsole/VncConsole.js

@@ -95,11 +96,11 @@ class VncConsole extends React.Component {
    this.props.onSecurityFailure(e);
  };

-  removeEventListeners() {
+  removeEventListeners = () => {


Because of context.

mareklibra · 2019-02-19T13:10:32Z

packages/patternfly-3/react-console/src/VncConsole/VncConsole.js

-          {status}
-          {this.novncStaticComponent}
-        </Toolbar.Results>
+        {portalToolbarTo ? (


as commented above

bond95 · 2019-03-22T13:04:56Z

@mareklibra please, review.

jeff-phillips-18 · 2019-05-17T20:11:51Z

@mareklibra Could you please take another look?

stale · 2019-07-16T20:29:22Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

bond95 · 2019-07-18T10:52:18Z

@priley86 could you, please, look at this PR?

priley86 · 2019-07-18T19:46:57Z

like other VncConsole PRs, this is hard to visualize. Can you possibly share an environment this is running w/ me? Or maybe it is easier to record a gif from your machine..

Also, do you plan to add PF4 variation? This is still using PF3 menus/buttons/etc..

bond95 · 2019-07-24T11:03:50Z

@priley86

like other VncConsole PRs, this is hard to visualize. Can you possibly share an environment this is running w/ me? Or maybe it is easier to record a gif from your machine..

Also, do you plan to add PF4 variation? This is still using PF3 menus/buttons/etc..

Yes, but in separate PR.

bond95 · 2019-07-26T12:58:02Z

@priley86 Could you, please, take another look?

priley86 · 2019-07-26T23:29:03Z

packages/patternfly-3/react-console/src/VncConsole/VncActions.js

+    return toolbar;
+  }
+  return (
+    document.getElementById(insertToolbarTo) && ReactDOM.createPortal(toolbar, document.getElementById(insertToolbarTo))


does this need to be a portal? We don't typically use portals for elements that will render in the typical DOM/page content area (they are more often used for appending modals/popovers/etc.).

I'm not currently using this downstream, so I can't say for sure, but you may be able to just expose it as a simple stateless component... we always break the JSX down into smaller reusable components if possible.

does this need to be a portal?

Yes, this component is depend on VncConsole component and for correct work should only be used inside of VncConsole. So from my point of view portals are the best solution.

priley86 · 2019-07-26T23:30:43Z

packages/patternfly-3/react-console/src/VncConsole/__snapshots__/VncActions.test.js.snap

->
-  <MenuItem
-    bsClass="dropdown"
+<div>


additional div?

No, div should be there.

patternfly-react/packages/patternfly-3/react-console/src/VncConsole/VncActions.js

Lines 17 to 28 in 223e582

const toolbar = (

<div>

<Dropdown bsStyle="default" title={textSendShortcut} id="console-send-shortcut" onClick={noop}>

<MenuItem eventKey="1" onClick={onCtrlAltDel}>

{textCtrlAltDel}

</MenuItem>

</Dropdown>

<Button bsStyle="default" onClick={onDisconnect}>

{textDisconnect}

</Button>

</div>

);

priley86

Conditionally approving. I don't want to block this effort since it is a 3rd party component being used w/ PF3. Added some comments above though.

All of this markup/CSS should be revisited w/ PF4 layouts/classes eventually if being put into a PF4 app. It seems like we are applying some additional divs/layout CSS which shouldn't be necessary in a PF4 context.

cc: @jeff-phillips-18 @mareklibra

thanks for the contribution @bond95 !

jeff-phillips-18 · 2019-07-30T14:16:36Z

packages/patternfly-3/react-console/src/VncConsole/VncActions.js

+  textSendShortcut: PropTypes.string,
+  textDisconnect: PropTypes.string,
+
+  insertToolbarTo: PropTypes.string


When would this be used? I don't see it being used here at all.
These propTypes should all be commented.

When would this be used? I don't see it being used here at all.

It is used in VncActions.

patternfly-react/packages/patternfly-3/react-console/src/VncConsole/VncActions.js

Lines 30 to 35 in 223e582

if (!insertToolbarTo) {

return toolbar;

}

return (

document.getElementById(insertToolbarTo) && ReactDOM.createPortal(toolbar, document.getElementById(insertToolbarTo))

);

These propTypes should all be commented.

Added comment.

patternfly-build · 2019-08-01T11:47:51Z

PatternFly-React preview: https://patternfly-react-pr-1402.surge.sh

…traction toolbar and console sc Added possibility to extract toolbar to any other component. Added scaling prop to VncConsole, and enhance `textConnecting` props to use node type as well. And fixed disconnecting.

jeff-phillips-18

Approving but agree with @priley86's comments.

Also, most of the console components do not show examples and while that is OK since they can't be demo'd without a backend, the components' properties should all be commented correctly for storybook info documentation and the components force included in the stories so that the docs for each component is provided.

This is not specific to this change as this is all pre-existing.

bond95 · 2019-08-08T12:15:16Z

@priley86 could you, please, look one more?

priley86 · 2019-08-08T15:29:04Z

merging - please be sure to file any follow up issues after testing downstream further.

thanks @bond95 !

bond95 force-pushed the vnc-console-enhacement branch from a4d81f7 to 9f1014b Compare February 19, 2019 09:59

mareklibra suggested changes Feb 19, 2019

View reviewed changes

bond95 force-pushed the vnc-console-enhacement branch from 9f1014b to 223e582 Compare February 19, 2019 16:17

bond95 mentioned this pull request Feb 22, 2019

Added NoVNC console page oVirt/ovirt-web-ui#956

Merged

dgutride added the PF3 label May 14, 2019

dlabaj assigned jeff-phillips-18 May 17, 2019

stale bot added the wontfix label Jul 16, 2019

stale bot removed the wontfix label Jul 18, 2019

priley86 reviewed Jul 26, 2019

View reviewed changes

priley86 previously approved these changes Jul 26, 2019

View reviewed changes

jeff-phillips-18 reviewed Jul 30, 2019

View reviewed changes

bond95 dismissed priley86’s stale review via c57707d August 1, 2019 11:42

bond95 force-pushed the vnc-console-enhacement branch from 223e582 to c57707d Compare August 1, 2019 11:42

bond95 force-pushed the vnc-console-enhacement branch from c57707d to 65fa453 Compare August 1, 2019 13:19

jeff-phillips-18 approved these changes Aug 1, 2019

View reviewed changes

priley86 approved these changes Aug 8, 2019

View reviewed changes

priley86 merged commit 122d68a into patternfly:master Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(VncConsole): More possibilities to customize component #1402

feat(VncConsole): More possibilities to customize component #1402

bond95 commented Feb 18, 2019 •

edited

Loading

mareklibra Feb 19, 2019

bond95 Feb 19, 2019

mareklibra Feb 19, 2019

bond95 Feb 19, 2019

priley86 Jul 18, 2019

mareklibra Feb 19, 2019

bond95 Feb 19, 2019

mareklibra Feb 19, 2019

bond95 Feb 19, 2019

mareklibra Feb 19, 2019

bond95 Feb 19, 2019

mareklibra Feb 19, 2019

bond95 Feb 19, 2019

mareklibra Feb 19, 2019

bond95 commented Mar 22, 2019

jeff-phillips-18 commented May 17, 2019

stale bot commented Jul 16, 2019

bond95 commented Jul 18, 2019

priley86 commented Jul 18, 2019

bond95 commented Jul 24, 2019 •

edited

Loading

bond95 commented Jul 26, 2019 •

edited

Loading

priley86 Jul 26, 2019 •

edited

Loading

bond95 Aug 1, 2019 •

edited

Loading

priley86 Jul 26, 2019

bond95 Aug 1, 2019

priley86 left a comment •

edited

Loading

jeff-phillips-18 Jul 30, 2019

bond95 Aug 1, 2019

bond95 Aug 1, 2019

patternfly-build commented Aug 1, 2019

jeff-phillips-18 left a comment

bond95 commented Aug 8, 2019

priley86 commented Aug 8, 2019

	const toolbar = (
	<div>
	<Dropdown bsStyle="default" title={textSendShortcut} id="console-send-shortcut" onClick={noop}>
	<MenuItem eventKey="1" onClick={onCtrlAltDel}>
	{textCtrlAltDel}
	</MenuItem>
	</Dropdown>
	<Button bsStyle="default" onClick={onDisconnect}>
	{textDisconnect}
	</Button>
	</div>
	);

	if (!insertToolbarTo) {
	return toolbar;
	}
	return (
	document.getElementById(insertToolbarTo) && ReactDOM.createPortal(toolbar, document.getElementById(insertToolbarTo))
	);

feat(VncConsole): More possibilities to customize component #1402

feat(VncConsole): More possibilities to customize component #1402

Conversation

bond95 commented Feb 18, 2019 • 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

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

bond95 commented Mar 22, 2019

jeff-phillips-18 commented May 17, 2019

stale bot commented Jul 16, 2019

bond95 commented Jul 18, 2019

priley86 commented Jul 18, 2019

bond95 commented Jul 24, 2019 • edited Loading

bond95 commented Jul 26, 2019 • edited Loading

priley86 Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

bond95 Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

patternfly-build commented Aug 1, 2019

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

bond95 commented Aug 8, 2019

priley86 commented Aug 8, 2019

bond95 commented Feb 18, 2019 •

edited

Loading

bond95 commented Jul 24, 2019 •

edited

Loading

bond95 commented Jul 26, 2019 •

edited

Loading

priley86 Jul 26, 2019 •

edited

Loading

bond95 Aug 1, 2019 •

edited

Loading

priley86 left a comment •

edited

Loading