Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 38 additions & 9 deletions packages/patternfly-3/react-console/src/VncConsole/VncActions.js
Original file line number Diff line number Diff line change
@@ -1,30 +1,59 @@
import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import { MenuItem, Button, helpers } from 'patternfly-react';

const { Dropdown } = Button;
const { noop } = helpers;

const VncActions = ({ textSendShortcut, textCtrlAltDel, onCtrlAltDel }) => (
<Dropdown bsStyle="default" title={textSendShortcut} id="console-send-shortcut" onClick={noop}>
<MenuItem eventKey="1" onClick={onCtrlAltDel}>
{textCtrlAltDel}
</MenuItem>
</Dropdown>
);
const VncActions = ({
textSendShortcut,
textCtrlAltDel,
textDisconnect,
onCtrlAltDel,
onDisconnect,
insertToolbarTo
}) => {
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))
Copy link
Member

@priley86 priley86 Jul 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bond95 bond95 Aug 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
};

VncActions.propTypes = {
onDisconnect: PropTypes.func.isRequired,
onCtrlAltDel: PropTypes.func,

textCtrlAltDel: PropTypes.string,
textSendShortcut: PropTypes.string
textSendShortcut: PropTypes.string,
textDisconnect: PropTypes.string,

insertToolbarTo: PropTypes.string // id of element where VncAction should be inserted
};

VncActions.defaultProps = {
onCtrlAltDel: noop,

textCtrlAltDel: 'Ctrl+Alt+Del',
textSendShortcut: 'Send Key'
textSendShortcut: 'Send Key',
textDisconnect: 'Disconnect',

insertToolbarTo: ''
};

export default VncActions;
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,30 @@ import { shallow, mount } from 'enzyme';
import VncActions from './VncActions';

test('placeholder render test', () => {
const view = shallow(<VncActions />);
const view = shallow(<VncActions onDisconnect={jest.fn()} />);
expect(view).toMatchSnapshot();
});

test('VncActions renders correctly component hierarchy', () => {
const view = shallow(
<VncActions textSendShortcut="My Send Shortcut description" textCtrlAltDel="foobar" onCtrlAltDel={jest.fn()} />
<VncActions
textSendShortcut="My Send Shortcut description"
textCtrlAltDel="foobar"
onCtrlAltDel={jest.fn()}
onDisconnect={jest.fn()}
/>
);
expect(view).toMatchSnapshot();
});

test('VncActions renders correctly html', () => {
const view = shallow(
<VncActions textSendShortcut="My Send Shortcut description" textCtrlAltDel="foobar" onCtrlAltDel={jest.fn()} />
<VncActions
textSendShortcut="My Send Shortcut description"
textCtrlAltDel="foobar"
onCtrlAltDel={jest.fn()}
onDisconnect={jest.fn()}
/>
);
expect(view.html()).toMatchSnapshot();
});
Expand All @@ -30,6 +40,7 @@ test('VncActions calls ctrl+alt+del action', () => {
textSendShortcut="My Send Shortcut description"
textCtrlAltDel="CtrlAltDel"
onCtrlAltDel={onCtrlAltDel}
onDisconnect={jest.fn()}
/>
);

Expand Down
58 changes: 44 additions & 14 deletions packages/patternfly-3/react-console/src/VncConsole/VncConsole.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class VncConsole extends React.Component {
path,
encrypt,
resizeSession,
scaleViewport,
viewOnly,
shared,
credentials,
Expand All @@ -53,7 +54,7 @@ class VncConsole extends React.Component {
this.rfb = new RFB(this.novncElem, url, options);
this.addEventListeners();
this.rfb.viewOnly = viewOnly;
this.rfb.scaleViewport = false; // if the remote session is smaller than HTML container, the view will be centered
this.rfb.scaleViewport = scaleViewport;
this.rfb.resizeSession = resizeSession;
} catch (e) {
onInitFailed && onInitFailed(e);
Expand All @@ -62,17 +63,17 @@ class VncConsole extends React.Component {
}

componentWillUnmount() {
this.removeEventListeners();
this.disconnect();
this.removeEventListeners();
this.rfb = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

disconnect() {
disconnect = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because otherwise method doesn't get this context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

if (!this.rfb) {
return;
}
this.rfb.disconnect();
this.rfb = undefined;
}
};

onConnected = () => {
this.setState({ status: CONNECTED });
Expand All @@ -95,11 +96,11 @@ class VncConsole extends React.Component {
this.props.onSecurityFailure(e);
};

removeEventListeners() {
removeEventListeners = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of context.

this.rfb.removeEventListener('connect', this.onConnected);
this.rfb.removeEventListener('disconnect', this.onDisconnected);
this.rfb.removeEventListener('securityfailure', this.onSecurityFailure);
}
};

setNovncElem = e => {
this.novncElem = e;
Expand All @@ -113,17 +114,27 @@ class VncConsole extends React.Component {
};

render() {
const { textDisconnected, textConnecting, textSendShortcut, textCtrlAltDel } = this.props;
const {
textDisconnected,
textConnecting,
textSendShortcut,
textCtrlAltDel,
textDisconnect,
portalToolbarTo
} = this.props;

let status = null;
let rightContent = null;
switch (this.state.status) {
case CONNECTED:
rightContent = (
<VncActions
portalToolbarTo={portalToolbarTo}
onCtrlAltDel={this.onCtrlAltDel}
textSendShortcut={textSendShortcut}
textCtrlAltDel={textCtrlAltDel}
textDisconnect={textDisconnect}
onDisconnect={this.disconnect}
/>
);
break;
Expand All @@ -143,11 +154,21 @@ class VncConsole extends React.Component {
return (
<div className={classNames('vnc-console', this.props.topClassName)}>
{this.props.children}
<Toolbar.RightContent>{rightContent}</Toolbar.RightContent>
<Toolbar.Results>
{status}
{this.novncStaticComponent}
</Toolbar.Results>
{portalToolbarTo ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as commented above

<React.Fragment>
{rightContent}
{status}
{this.novncStaticComponent}
</React.Fragment>
) : (
<React.Fragment>
<Toolbar.RightContent>{rightContent}</Toolbar.RightContent>
<Toolbar.Results>
{status}
{this.novncStaticComponent}
</Toolbar.Results>
</React.Fragment>
)}
</div>
);
}
Expand All @@ -163,20 +184,26 @@ VncConsole.propTypes = {
path: PropTypes.string /** host:port/path */,
encrypt: PropTypes.bool /** For all following, see: https://github.com/novnc/noVNC/blob/master/docs/API.md */,
resizeSession: PropTypes.bool /** Change remote session size according to local HTML container */,
scaleViewport: PropTypes.bool /** Scale session size according to parent HTML container */,
viewOnly: PropTypes.bool,
shared: PropTypes.bool,
credentials: PropTypes.object /** { username: '', password: '', target: ''} */,
repeaterID: PropTypes.string,
vncLogging: PropTypes.string /** log-level for noVNC */,
portalToolbarTo: PropTypes.string,

topClassName: PropTypes.string /** Enable customization */,

onDisconnected: PropTypes.func /** Callback. VNC server disconnected. */,
onInitFailed: PropTypes.func /** Initialization of RFB failed */,
onSecurityFailure: PropTypes.func /** Handshake failed */,

textConnecting: PropTypes.string /** For localization */,
textConnecting: PropTypes.oneOfType([
PropTypes.string,
PropTypes.node
]) /** For localization and better integration */,
textDisconnected: PropTypes.string,
textDisconnect: PropTypes.string,
textSendShortcut: PropTypes.string,
textCtrlAltDel: PropTypes.string
};
Expand All @@ -188,11 +215,13 @@ VncConsole.defaultProps = {
path: '',
encrypt: false,
resizeSession: true,
scaleViewport: false,
viewOnly: false,
shared: false,
credentials: undefined,
repeaterID: '',
vncLogging: 'warn',
portalToolbarTo: '',

topClassName: '',

Expand All @@ -202,6 +231,7 @@ VncConsole.defaultProps = {

textConnecting: 'Connecting',
textDisconnected: 'Disconnected',
textDisconnect: 'Disconnect',
textSendShortcut: undefined /** Default value defined in VncActions */,
textCtrlAltDel: undefined /** Default value defined in VncActions */
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,43 +1,67 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`VncActions renders correctly component hierarchy 1`] = `
<DropdownButton
bsStyle="default"
id="console-send-shortcut"
onClick={[Function]}
title="My Send Shortcut description"
>
<MenuItem
bsClass="dropdown"
<div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional div?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, div should be there.

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>
);

<DropdownButton
bsStyle="default"
id="console-send-shortcut"
onClick={[Function]}
title="My Send Shortcut description"
>
<MenuItem
bsClass="dropdown"
disabled={false}
divider={false}
eventKey="1"
header={false}
onClick={[MockFunction]}
>
foobar
</MenuItem>
</DropdownButton>
<Button
active={false}
block={false}
bsClass="btn"
bsStyle="default"
disabled={false}
divider={false}
eventKey="1"
header={false}
onClick={[MockFunction]}
>
foobar
</MenuItem>
</DropdownButton>
Disconnect
</Button>
</div>
`;

exports[`VncActions renders correctly html 1`] = `"<div class=\\"dropdown btn-group btn-group-default\\"><button id=\\"console-send-shortcut\\" role=\\"button\\" aria-haspopup=\\"true\\" aria-expanded=\\"false\\" type=\\"button\\" class=\\"dropdown-toggle btn btn-default\\">My Send Shortcut description <span class=\\"caret\\"></span></button><ul role=\\"menu\\" class=\\"dropdown-menu\\" aria-labelledby=\\"console-send-shortcut\\"><li role=\\"presentation\\" class=\\"\\"><a role=\\"menuitem\\" tabindex=\\"-1\\" href=\\"#\\">foobar</a></li></ul></div>"`;
exports[`VncActions renders correctly html 1`] = `"<div><div class=\\"dropdown btn-group btn-group-default\\"><button id=\\"console-send-shortcut\\" role=\\"button\\" aria-haspopup=\\"true\\" aria-expanded=\\"false\\" type=\\"button\\" class=\\"dropdown-toggle btn btn-default\\">My Send Shortcut description <span class=\\"caret\\"></span></button><ul role=\\"menu\\" class=\\"dropdown-menu\\" aria-labelledby=\\"console-send-shortcut\\"><li role=\\"presentation\\" class=\\"\\"><a role=\\"menuitem\\" tabindex=\\"-1\\" href=\\"#\\">foobar</a></li></ul></div><button type=\\"button\\" class=\\"btn btn-default\\">Disconnect</button></div>"`;

exports[`placeholder render test 1`] = `
<DropdownButton
bsStyle="default"
id="console-send-shortcut"
onClick={[Function]}
title="Send Key"
>
<MenuItem
bsClass="dropdown"
disabled={false}
divider={false}
eventKey="1"
header={false}
<div>
<DropdownButton
bsStyle="default"
id="console-send-shortcut"
onClick={[Function]}
title="Send Key"
>
<MenuItem
bsClass="dropdown"
disabled={false}
divider={false}
eventKey="1"
header={false}
onClick={[Function]}
>
Ctrl+Alt+Del
</MenuItem>
</DropdownButton>
<Button
active={false}
block={false}
bsClass="btn"
bsStyle="default"
disabled={false}
onClick={[MockFunction]}
>
Ctrl+Alt+Del
</MenuItem>
</DropdownButton>
Disconnect
</Button>
</div>
`;