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

Bug 1829549: fix resizing issues with cloud shell terminal drawer #5224

Merged
merged 1 commit into from
May 8, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@ const getMastheadHeight = (): number => {
};

const CloudShellDrawer: React.FC<CloudShellDrawerProps> = ({ children, onClose }) => {
const [height, setHeight] = React.useState(365);
const [expanded, setExpanded] = React.useState<boolean>(true);
const onMRButtonClick = (expandedState: boolean) => {
setExpanded(!expandedState);
};
const handleChange = (openState: boolean, resizeHeight: number) => {
const handleChange = (openState: boolean) => {
setExpanded(openState);
setHeight(resizeHeight);
};
const header = (
<Flex style={{ flexGrow: 1 }}>
Expand Down Expand Up @@ -65,7 +63,7 @@ const CloudShellDrawer: React.FC<CloudShellDrawerProps> = ({ children, onClose }
return (
<Drawer
open={expanded}
height={height}
defaultHeight={365}
header={header}
maxHeight={`calc(100vh - ${getMastheadHeight()}px)`}
onChange={handleChange}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
height: 100%;
color: var(--pf-global--Color--light-100);
overflow: hidden;
position: absolute;

& > iframe {
height: 100%;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.ocs-draggable-core-iframe-fix {
& iframe {
pointer-events: none !important;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import * as React from 'react';
import { DraggableCore, DraggableEvent, DraggableData } from 'react-draggable';

import './DraggableCoreIFrameFix.scss';

const DraggableCoreIFrameFix: React.FC<React.ComponentProps<typeof DraggableCore>> = ({
onStart,
onStop,
...other
}) => {
const onStartFn =
// rule is inconsistent with typescript return type
// eslint-disable-next-line consistent-return
(e: DraggableEvent, data: DraggableData): false | void => {
document.body.classList.add('ocs-draggable-core-iframe-fix');
if (onStart) {
return onStart(e, data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to return here? we can just call the onStart function without returning it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API from react-draggable is to return false | void this ensures that we allow the onStart provided to have a return value.

}
};

const onStopFn =
// rule is inconsistent with typescript return type
// eslint-disable-next-line consistent-return
(e: DraggableEvent, data: DraggableData): false | void => {
document.body.classList.remove('ocs-draggable-core-iframe-fix');
if (onStop) {
return onStop(e, data);
}
};

return <DraggableCore {...other} onStart={onStartFn} onStop={onStopFn} />;
};

export default DraggableCoreIFrameFix;
89 changes: 55 additions & 34 deletions frontend/packages/console-shared/src/components/drawer/Drawer.tsx
Original file line number Diff line number Diff line change
@@ -1,26 +1,29 @@
import * as React from 'react';
import { CSSTransition } from 'react-transition-group';
import { DraggableData, DraggableCore } from 'react-draggable';
import { DraggableEvent } from 'react-draggable';
import DraggableCoreIFrameFix from './DraggableCoreIFrameFix';
import './Drawer.scss';

type DrawerProps = {
/**
* Height of the drawer,
* should be set when used as controlled component with onChange callback
* Controlled height of the drawer.
* Should be set when used as controlled component with onChange callback.
*/
height?: number;
/**
* Default Value: 300
* default height of the drawer component,
* should be set when used as Uncontrolled component
* Uncontrolled default height of the drawer.
*/
defaultHeight?: number;
/**
* Defines the minimized state of drawer
* false: Height is minimum height (minimized state)
* true: Height is greater than minimum height
* Toggles controlled open state.
*/
open?: boolean;
/**
* Default Value: true
* Uncontrolled open state of the drawer on first render.
*/
defaultOpen?: boolean;
/**
* Maximum height drawer can be resized to.
*/
Expand Down Expand Up @@ -53,61 +56,79 @@ const useSize = <T extends HTMLElement>(): [number, (element: T) => void] => {
return [height, callback];
};

// get the pageX value from a mouse or touch event
const getPageY = (e: DraggableEvent): number =>
(e as MouseEvent).pageY ?? (e as TouchEvent).touches?.[0]?.pageY;

const Drawer: React.FC<DrawerProps> = ({
children,
defaultHeight = 300,
height,
maxHeight = '100%',
open = true,
open,
defaultOpen = true,
resizable = false,
header,
onChange,
}) => {
const drawerRef = React.useRef<HTMLDivElement>();
const [heightState, setHeightState] = React.useState(defaultHeight);
const lastObservedHeightRef = React.useRef<number>(0);
const [openState, setOpenState] = React.useState(defaultOpen);
const lastObservedHeightRef = React.useRef<number>();
const startRef = React.useRef<number>();
const [minHeight, headerRef] = useSize<HTMLDivElement>();
const resizeHeight = height ?? heightState;
const minimumHeight = minHeight ?? 0;
const handleDrag = (e: MouseEvent, data: DraggableData) => {
const newHeight = resizeHeight - data.y;

// merge controlled and uncontrolled states
const currentOpen = open ?? openState;
const currentHeight = height ?? heightState;

const setHeight = (drawerHeight: number, forceOpen?: boolean) => {
const newHeight = Math.max(drawerHeight, minimumHeight);
const newOpen = forceOpen ?? newHeight > minimumHeight;
setHeightState(newHeight);
setOpenState(newOpen);
if (onChange) {
if (newHeight > minimumHeight) {
onChange(true, newHeight);
} else {
onChange(false, minimumHeight);
}
} else {
setHeightState(newHeight);
onChange(newOpen, newHeight);
}
};

const handleResizeStart = (e: MouseEvent, data: DraggableData) => {
lastObservedHeightRef.current = resizeHeight;
const newHeight = resizeHeight - data.y;
// if the drawer is in minimized state and drag started then
// reset the lastObservedHeight to minimumHeight
if (onChange && !open && newHeight > minimumHeight) {
onChange(false, minimumHeight);
const handleDrag = (e: DraggableEvent) => {
setHeight(startRef.current - getPageY(e));
};

const handleResizeStart = (e: DraggableEvent) => {
lastObservedHeightRef.current = currentHeight;
// always start with actual drawer height
const drawerHeight = drawerRef.current?.offsetHeight || currentHeight;
startRef.current = drawerHeight + getPageY(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use DraggableData instead of getPageY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DraggableData doesn't work for us once we hit max height. if you look at the height that's computed in master when you start to drag up beyond the masthead, you'll see that the height quickly increases by hundreds and then thousands. Which is incorrect. By using pageY we are always computing a height value relative to the starting height rather than relative to the last event.

if (drawerHeight !== currentHeight) {
setHeight(drawerHeight);
}
};

const handleResizeStop = (e: MouseEvent, data: DraggableData) => {
const newHeight = resizeHeight - data.y;
if (onChange && newHeight <= minimumHeight) {
onChange(false, lastObservedHeightRef.current);
const handleResizeStop = () => {
if (currentHeight <= minimumHeight) {
setHeight(lastObservedHeightRef.current, false);
}
};

const draggable = resizable && (
<DraggableCore onDrag={handleDrag} onStart={handleResizeStart} onStop={handleResizeStop}>
<DraggableCoreIFrameFix
onDrag={handleDrag}
onStart={handleResizeStart}
onStop={handleResizeStop}
>
<div className="ocs-drawer__drag-handle" />
</DraggableCore>
</DraggableCoreIFrameFix>
);
return (
<CSSTransition appear in timeout={225} classNames="ocs-drawer">
<div
ref={drawerRef}
className="ocs-drawer"
style={{
height: open ? resizeHeight : minimumHeight,
height: currentOpen ? currentHeight : minimumHeight,
maxHeight,
minHeight: minimumHeight,
}}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import * as React from 'react';
import { shallow } from 'enzyme';
import DraggableCoreIFrameFix from '../DraggableCoreIFrameFix';
import { DraggableCore, DraggableEvent, DraggableData } from 'react-draggable';

describe('DraggableCoreIFrameFix', () => {
it('should execute handlers and apply fix class', () => {
const onStart = jest.fn();
const onStop = jest.fn();
const event = {} as DraggableEvent;
const data = {} as DraggableData;
const wrapper = shallow(<DraggableCoreIFrameFix onStart={onStart} onStop={onStop} />);

wrapper
.find(DraggableCore)
.props()
.onStart(event, data);
expect(document.body.className).toBe('ocs-draggable-core-iframe-fix');

wrapper
.find(DraggableCore)
.props()
.onStop(event, data);
expect(document.body.className).toBe('');

expect(onStart).toHaveBeenCalledWith(event, data);
expect(onStop).toHaveBeenCalledWith(event, data);
});
});
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as React from 'react';
import { shallow } from 'enzyme';
import { DraggableData } from 'react-draggable';
import Drawer from '../Drawer';
import { DraggableCore } from 'react-draggable';
import DraggableCoreIFrameFix from '../DraggableCoreIFrameFix';

describe('DrawerComponent', () => {
it('should exist', () => {
Expand All @@ -10,31 +11,44 @@ describe('DrawerComponent', () => {
});

it('should have default values', () => {
const content = 'This is content';
const wrapper = shallow(
<Drawer>
<p id="dummy-content">{content}</p>
<p id="dummy-content" />
</Drawer>,
);
expect(wrapper.find(DraggableCore).exists()).toBe(false);
expect(wrapper.find(DraggableCoreIFrameFix).exists()).toBe(false);
const style = wrapper.find('.ocs-drawer').prop('style');
expect(style.height).toEqual(300);
expect(style.height).toBe(300);
expect(wrapper.find('#dummy-content')).toHaveLength(1);
});

it('should be resizable and height 300', () => {
const wrapper = shallow(<Drawer resizable />);
expect(wrapper.find(DraggableCore).exists()).toBe(true);
expect(wrapper.find('.ocs-drawer').prop('style').height).toEqual(300);
expect(wrapper.find(DraggableCoreIFrameFix).exists()).toBe(true);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(300);
});

it('should support initially closed', () => {
let wrapper = shallow(<Drawer defaultOpen={false} />);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(0);
wrapper = shallow(<Drawer open={false} />);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(0);
});

it('should support initial height', () => {
let wrapper = shallow(<Drawer defaultHeight={100} />);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(100);
wrapper = shallow(<Drawer height={100} />);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(100);
});

it('should have maximumHeight', () => {
const height = `calc(100vh - 10%)`;
const wrapper = shallow(<Drawer maxHeight={height} />);
expect(wrapper.find('.ocs-drawer').prop('style').maxHeight).toEqual(height);
expect(wrapper.find('.ocs-drawer').prop('style').maxHeight).toBe(height);
const nextHeight = 950;
wrapper.setProps({ maxHeight: nextHeight });
expect(wrapper.find('.ocs-drawer').prop('style').maxHeight).toEqual(nextHeight);
expect(wrapper.find('.ocs-drawer').prop('style').maxHeight).toBe(nextHeight);
});

it('should have header', () => {
Expand All @@ -47,7 +61,7 @@ describe('DrawerComponent', () => {
.find('.ocs-drawer__header')
.children()
.html(),
).toEqual('<p>This is header</p>');
).toBe('<p>This is header</p>');
});

it('should render children', () => {
Expand All @@ -57,17 +71,46 @@ describe('DrawerComponent', () => {
<p id="dummy-content">{content}</p>
</Drawer>,
);
expect(wrapper.find('#dummy-content').exists()).toEqual(true);
expect(wrapper.find('#dummy-content').text()).toEqual(content);
expect(wrapper.find('#dummy-content').exists()).toBe(true);
});

it('should be set to minimum height when open is set to false and height if open is set to true', () => {
const wrapper = shallow(<Drawer defaultHeight={500} open={false} />);
const style = wrapper.find('.ocs-drawer').prop('style');
expect(style.height).toEqual(0);
expect(style.minHeight).toEqual(0);
expect(style.maxHeight).toEqual('100%');
expect(style.height).toBe(0);
expect(style.minHeight).toBe(0);
expect(style.maxHeight).toBe('100%');
wrapper.setProps({ open: true, defaultHeight: 500 });
expect(wrapper.find('.ocs-drawer').prop('style').height).toEqual(500);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(500);
});

it('should handle resizing', () => {
const data = {} as DraggableData;
const onChange = jest.fn();
const wrapper = shallow(<Drawer resizable defaultHeight={100} onChange={onChange} />);
wrapper
.find(DraggableCoreIFrameFix)
.props()
.onStart({ pageY: 500 } as any, data);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(100);
wrapper
.find(DraggableCoreIFrameFix)
.props()
.onDrag({ pageY: 550 } as any, data);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(50);
expect(onChange).toHaveBeenLastCalledWith(true, 50);
onChange.mockClear();
wrapper
.find(DraggableCoreIFrameFix)
.props()
.onDrag({ pageY: 700 } as any, data);
expect(wrapper.find('.ocs-drawer').prop('style').height).toBe(0);
expect(onChange).toHaveBeenLastCalledWith(false, 0);
onChange.mockClear();
wrapper
.find(DraggableCoreIFrameFix)
.props()
.onStop({} as any, data);
expect(onChange).toHaveBeenLastCalledWith(false, 100);
});
});