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.

Sign up for GitHub

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

Jump to bottom

[Lens] Keyboard-selected items follow user traversal of drop zones #90546

Merged
merged 15 commits into from
Feb 15, 2021
Merged

[Lens] Keyboard-selected items follow user traversal of drop zones #90546

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e19f9ad
adding a ghost image
mbondyra Feb 6, 2021
a2f1c14
fix safari fullscreen
mbondyra Feb 9, 2021
97c3393
Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…
kibanamachine Feb 9, 2021
464f71c
Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…
kibanamachine Feb 9, 2021
548b774
Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…
kibanamachine Feb 10, 2021
45dbf89
show ghost instead of noGhost
mbondyra Feb 10, 2021
1c2d223
Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…
mbondyra Feb 10, 2021
82c7c22
Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…
kibanamachine Feb 11, 2021
c13cfd0
cr corrections
mbondyra Feb 11, 2021
a84ed51
Merge commit '6bd0a7fcc5f477b75ca173ee0de11ebcd2898f4f' into lens/dnd…
mbondyra Feb 11, 2021
7e2d40d
removing commented code
mbondyra Feb 11, 2021
3eb52e6
remvoing commented code
mbondyra Feb 11, 2021
6f60e55
remvoing commented code
mbondyra Feb 11, 2021
2cef3ea
Update x-pack/plugins/lens/public/drag_drop/drag_drop.scss
mbondyra Feb 11, 2021
1f6aa1a
Merge branch 'master' into lens/dnd_add_ghost_image_to_keyboard_navig…
kibanamachine Feb 15, 2021
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
1 change: 1 addition & 0 deletions x-pack/plugins/lens/public/drag_drop/__snapshots__/drag_drop.test.tsx.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 28 additions & 5 deletions x-pack/plugins/lens/public/drag_drop/drag_drop.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,19 @@
transition: background-color $euiAnimSpeedFast ease-in-out, border-color $euiAnimSpeedFast ease-in-out;
}

.lnsDragDrop_ghost {
@include lnsDraggable;
border: $euiBorderWidthThin dashed $euiBorderColor;
position: absolute !important; // sass-lint:disable-line no-important
top:0;
width: 100%;
left:0;
opacity:.9;
transform: translate(-25px, -30px);
z-index: $euiZLevel2;
pointer-events: none;
}

// Draggable item
.lnsDragDrop-isDraggable {
@include lnsDraggable;
Expand Down Expand Up @@ -87,11 +100,6 @@
z-index: $euiZLevel1;
}

// Draggable item when it is moving
.lnsDragDrop-isHidden {
opacity: 0;
}

.lnsDragDrop__keyboardHandler {
top: 0;
position: absolute;
Expand All @@ -105,3 +113,18 @@
pointer-events: none;
}
}

// Draggable item when it is moving
.lnsDragDrop-isHidden {
opacity: 0;
}

.lnsDragDrop-isHidden-noFocus {
opacity: 0;
.lnsDragDrop__keyboardHandler {
&:focus,
&:focus-within {
animation: none;
}
}
}
63 changes: 41 additions & 22 deletions x-pack/plugins/lens/public/drag_drop/drag_drop.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
DragContextState,
ReorderProvider,
DragDropIdentifier,
DraggingIdentifier,
DropTargets,
} from './providers';
import { act } from 'react-dom/test-utils';
Expand Down Expand Up @@ -81,7 +82,7 @@ describe('DragDrop', () => {
const component = mount(
<ChildDragDropProvider
{...defaultContext}
dragging={value}
dragging={{ ...value, ghost: <button>Hello!</button> }}
setDragging={setDragging}
setA11yMessage={setA11yMessage}
>
Expand All @@ -96,7 +97,7 @@ describe('DragDrop', () => {
jest.runAllTimers();

expect(dataTransfer.setData).toBeCalledWith('text', 'hello');
expect(setDragging).toBeCalledWith(value);
expect(setDragging).toBeCalledWith({ ...value, ghost: <button>Hello!</button> });
expect(setA11yMessage).toBeCalledWith('Lifted hello');
});

Expand All @@ -109,7 +110,7 @@ describe('DragDrop', () => {
const component = mount(
<ChildDragDropProvider
{...defaultContext}
dragging={{ id: '2', humanData: { label: 'label1' } }}
dragging={{ id: '2', humanData: { label: 'label1' }, ghost: <button>Hello!</button> }}
setDragging={setDragging}
>
<DragDrop onDrop={onDrop} dropType="field_add" value={value} order={[2, 0, 1, 0]}>
Expand All @@ -125,7 +126,10 @@ describe('DragDrop', () => {
expect(preventDefault).toBeCalled();
expect(stopPropagation).toBeCalled();
expect(setDragging).toBeCalledWith(undefined);
expect(onDrop).toBeCalledWith({ id: '2', humanData: { label: 'label1' } }, 'field_add');
expect(onDrop).toBeCalledWith(
{ id: '2', humanData: { label: 'label1' }, ghost: <button>Hello!</button> },
'field_add'
);
});

test('drop function is not called on dropType undefined', async () => {
Expand All @@ -137,7 +141,7 @@ describe('DragDrop', () => {
const component = mount(
<ChildDragDropProvider
{...defaultContext}
dragging={{ id: 'hi', humanData: { label: 'label1' } }}
dragging={{ id: 'hi', humanData: { label: 'label1' }, ghost: <button>Hello!</button> }}
setDragging={setDragging}
>
<DragDrop onDrop={onDrop} dropType={undefined} value={value} order={[2, 0, 1, 0]}>
Expand Down Expand Up @@ -175,7 +179,7 @@ describe('DragDrop', () => {

test('items that has dropType=undefined get special styling when another item is dragged', () => {
const component = mount(
<ChildDragDropProvider {...defaultContext} dragging={value}>
<ChildDragDropProvider {...defaultContext} dragging={{ ...value, ghost: <div /> }}>
<DragDrop value={value} draggable={true} order={[2, 0, 1, 0]}>
<button>Hello!</button>
</DragDrop>
Expand All @@ -194,7 +198,9 @@ describe('DragDrop', () => {
});

test('additional styles are reflected in the className until drop', () => {
let dragging: { id: '1'; humanData: { label: 'label1' } } | undefined;
let dragging:
| { id: '1'; humanData: { label: 'label1' }; ghost: React.ReactElement }
| undefined;
const getAdditionalClassesOnEnter = jest.fn().mockReturnValue('additional');
const getAdditionalClassesOnDroppable = jest.fn().mockReturnValue('droppable');
const setA11yMessage = jest.fn();
Expand All @@ -206,7 +212,7 @@ describe('DragDrop', () => {
dragging={dragging}
setA11yMessage={setA11yMessage}
setDragging={() => {
dragging = { id: '1', humanData: { label: 'label1' } };
dragging = { id: '1', humanData: { label: 'label1' }, ghost: <div>Hello</div> };
}}
setActiveDropTarget={(val) => {
activeDropTarget = { activeDropTarget: val };
Expand Down Expand Up @@ -246,7 +252,9 @@ describe('DragDrop', () => {
});

test('additional enter styles are reflected in the className until dragleave', () => {
let dragging: { id: '1'; humanData: { label: 'label1' } } | undefined;
let dragging:
| { id: '1'; humanData: { label: 'label1' }; ghost: React.ReactElement }
| undefined;
const getAdditionalClasses = jest.fn().mockReturnValue('additional');
const getAdditionalClassesOnDroppable = jest.fn().mockReturnValue('droppable');
const setActiveDropTarget = jest.fn();
Expand All @@ -256,7 +264,7 @@ describe('DragDrop', () => {
setA11yMessage={jest.fn()}
dragging={dragging}
setDragging={() => {
dragging = { id: '1', humanData: { label: 'label1' } };
dragging = { id: '1', humanData: { label: 'label1' }, ghost: <div /> };
}}
setActiveDropTarget={setActiveDropTarget}
activeDropTarget={
Expand Down Expand Up @@ -350,7 +358,7 @@ describe('DragDrop', () => {
<ChildDragDropProvider
{...{
...defaultContext,
dragging: items[0].value,
dragging: { ...items[0].value, ghost: <div /> },
setActiveDropTarget,
setA11yMessage,
activeDropTarget: {
Expand Down Expand Up @@ -427,7 +435,7 @@ describe('DragDrop', () => {
const registerDropTarget = jest.fn();
const baseContext = {
dragging,
setDragging: (val?: DragDropIdentifier) => {
setDragging: (val?: DraggingIdentifier) => {
dragging = val;
},
keyboardMode,
Expand Down Expand Up @@ -479,7 +487,11 @@ describe('DragDrop', () => {
test(`Reorderable group with lifted element renders properly`, () => {
const setA11yMessage = jest.fn();
const setDragging = jest.fn();
const component = mountComponent({ dragging: items[0], setDragging, setA11yMessage });
const component = mountComponent({
dragging: { ...items[0], ghost: <span>1</span> },
setDragging,
setA11yMessage,
});
act(() => {
component
.find('[data-test-subj="lnsDragDrop"]')
Expand All @@ -488,7 +500,7 @@ describe('DragDrop', () => {
jest.runAllTimers();
});

expect(setDragging).toBeCalledWith(items[0]);
expect(setDragging).toBeCalledWith({ ...items[0], ghost: <span>1</span> });
expect(setA11yMessage).toBeCalledWith('Lifted label1');
expect(
component
Expand All @@ -498,7 +510,7 @@ describe('DragDrop', () => {
});

test(`Reordered elements get extra styles to show the reorder effect when dragging`, () => {
const component = mountComponent({ dragging: items[0] });
const component = mountComponent({ dragging: { ...items[0], ghost: <div /> } });

act(() => {
component
Expand Down Expand Up @@ -545,7 +557,11 @@ describe('DragDrop', () => {
const setA11yMessage = jest.fn();
const setDragging = jest.fn();

const component = mountComponent({ dragging: items[0], setDragging, setA11yMessage });
const component = mountComponent({
dragging: { ...items[0], ghost: <div /> },
setDragging,
setA11yMessage,
});

component
.find('[data-test-subj="lnsDragDrop-reorderableDropLayer"]')
Expand All @@ -558,14 +574,14 @@ describe('DragDrop', () => {
);
expect(preventDefault).toBeCalled();
expect(stopPropagation).toBeCalled();
expect(onDrop).toBeCalledWith(items[0], 'reorder');
expect(onDrop).toBeCalledWith({ ...items[0], ghost: <div /> }, 'reorder');
});

test(`Keyboard Navigation: User cannot move an element outside of the group`, () => {
const setA11yMessage = jest.fn();
const setActiveDropTarget = jest.fn();
const component = mountComponent({
dragging: items[0],
dragging: { ...items[0], ghost: <div /> },
keyboardMode: true,
activeDropTarget: {
activeDropTarget: undefined,
Expand Down Expand Up @@ -594,7 +610,7 @@ describe('DragDrop', () => {
});
test(`Keyboard navigation: user can drop element to an activeDropTarget`, () => {
const component = mountComponent({
dragging: items[0],
dragging: { ...items[0], ghost: <div /> },
activeDropTarget: {
activeDropTarget: { ...items[2], dropType: 'reorder', onDrop },
dropTargetsByOrder: {
Expand All @@ -621,7 +637,10 @@ describe('DragDrop', () => {
test(`Keyboard Navigation: Doesn't call onDrop when movement is cancelled`, () => {
const setA11yMessage = jest.fn();
const onDropHandler = jest.fn();
const component = mountComponent({ dragging: items[0], setA11yMessage }, onDropHandler);
const component = mountComponent(
{ dragging: { ...items[0], ghost: <div /> }, setA11yMessage },
onDropHandler
);
const keyboardHandler = component.find('[data-test-subj="lnsDragDrop-keyboardHandler"]');
keyboardHandler.simulate('keydown', { key: 'Space' });
keyboardHandler.simulate('keydown', { key: 'Escape' });
Expand All @@ -640,7 +659,7 @@ describe('DragDrop', () => {
test(`Keyboard Navigation: Reordered elements get extra styles to show the reorder effect`, () => {
const setA11yMessage = jest.fn();
const component = mountComponent({
dragging: items[0],
dragging: { ...items[0], ghost: <div /> },
keyboardMode: true,
activeDropTarget: {
activeDropTarget: undefined,
Expand Down Expand Up @@ -704,7 +723,7 @@ describe('DragDrop', () => {
'2,0,1,1': { ...items[1], onDrop, dropType: 'reorder' },
},
}}
dragging={items[0]}
dragging={{ ...items[0], ghost: <div /> }}
setActiveDropTarget={setActiveDropTarget}
setA11yMessage={setA11yMessage}
>
Expand Down
35 changes: 32 additions & 3 deletions x-pack/plugins/lens/public/drag_drop/drag_drop.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ interface BaseProps {
* Order for keyboard dragging. This takes an array of numbers which will be used to order hierarchically
*/
order: number[];

/**
* don't display ghost image for the drop element
*/
noGhost?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I find negative props somewhat confusing in components. Maybe others do to?

Especially when passing in false you end up with a double negative when thinking it through and that can throw people (me, at least) for a loop.

Copy link
Contributor Author

@mbondyra mbondyra Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand and it makes sense. However, on the other side, this construction allows us to treat displaying a ghost as a default behaviour without passing extra parameter and that's what I was trying to do here. So for most components, I can do <DragDrop draggable .../> and I don't have to add ghost = {true} (or specify a default value in the component). I understand it's a bit on the edge when it comes to readability so I'd like to know the opinion of another person reviewing this 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong feeling here, but we could call the prop ghost and treat undefined as true:

{(ghost || typeof ghost === 'undefined') &&
      dragging?.ghost &&
      activeDropTarg...

}

/**
Expand Down Expand Up @@ -232,7 +237,7 @@ const DragInner = memo(function DragInner({

const currentTarget = e?.currentTarget;
setTimeout(() => {
setDragging(value);
setDragging({ ...value, ghost: children });
setA11yMessage(announce.lifted(value.humanData));
if (onDragStart) {
onDragStart(currentTarget);
Expand Down Expand Up @@ -278,8 +283,19 @@ const DragInner = memo(function DragInner({
: announce.noTarget()
);
};
const shouldShowGhostImageInstead =
isDragging &&
dragType === 'move' &&
keyboardMode &&
activeDropTarget?.activeDropTarget &&
activeDropTarget?.activeDropTarget.dropType !== 'reorder';
return (
<div className={className} data-test-subj={`lnsDragDrop_draggable-${value.humanData.label}`}>
<div
className={classNames(className, {
'lnsDragDrop-isHidden-noFocus': shouldShowGhostImageInstead,
})}
data-test-subj={`lnsDragDrop_draggable-${value.humanData.label}`}
>
<EuiScreenReaderOnly showOnFocus>
<button
aria-label={value.humanData.label}
Expand All @@ -305,6 +321,8 @@ const DragInner = memo(function DragInner({
}
} else if (key === keys.ESCAPE) {
if (isDragging) {
e.stopPropagation();
e.preventDefault();
dragEnd();
}
}
Expand All @@ -321,7 +339,8 @@ const DragInner = memo(function DragInner({
{React.cloneElement(children, {
'data-test-subj': dataTestSubj || 'lnsDragDrop',
className: classNames(children.props.className, 'lnsDragDrop', 'lnsDragDrop-isDraggable', {
'lnsDragDrop-isHidden': isDragging && dragType === 'move' && !keyboardMode,
'lnsDragDrop-isHidden':
(isDragging && dragType === 'move' && !keyboardMode) || shouldShowGhostImageInstead,
}),
draggable: true,
onDragEnd: dragEnd,
Expand All @@ -344,6 +363,7 @@ const DropInner = memo(function DropInner(props: DropInnerProps) {
isNotDroppable,
dragType = 'copy',
dropType,
noGhost,
keyboardMode,
activeDropTarget,
registerDropTarget,
Expand Down Expand Up @@ -431,6 +451,15 @@ const DropInner = memo(function DropInner(props: DropInnerProps) {
onDrop: drop,
draggable,
})}
{!noGhost &&
dragging?.ghost &&
activeDropTargetMatches &&
keyboardMode &&
dropType !== 'reorder'
? React.cloneElement(dragging.ghost, {
className: classNames('lnsDragDrop_ghost', children.props.className),
})
: null}
</>
);
});
Expand Down
Loading