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

[Ingest Pipelines Editor] First round of UX improvements #69381

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
114f141
First round of UX tweaks
jloleysens Jun 17, 2020
34e313f
Updated the on-failure pipeline description copy
jloleysens Jun 17, 2020
1328c24
Merge branch 'master' into ingest-pipelines/editor-dropzone-refinement
elasticmachine Jun 18, 2020
7edd5ba
Properly encode URI component pipeline names
jloleysens Jun 18, 2020
448c882
use xjson editor in flyout
jloleysens Jun 18, 2020
110e5cf
also hide the test flyout if we are editing a component
jloleysens Jun 18, 2020
b4ef7a8
add much stronger dimming effect when in edit mode
jloleysens Jun 18, 2020
9c44d87
also added dimming effect to moving state
jloleysens Jun 18, 2020
5326b25
remove box shadow if dimmed
jloleysens Jun 18, 2020
1017792
add tooltips to dropzones
jloleysens Jun 22, 2020
1c4e4f2
Merge branch 'master' of github.com:elastic/kibana into ingest-pipeli…
jloleysens Jun 22, 2020
fe68f96
fix CITs after master merge
jloleysens Jun 22, 2020
7f25e6a
Merge branch 'master' into ingest-pipelines/editor-dropzone-refinement
elasticmachine Jun 23, 2020
6ac957f
fix nested rendering of processors tree
jloleysens Jun 23, 2020
97096f1
Merge branch 'master' into ingest-pipelines/editor-dropzone-refinement
elasticmachine Jun 25, 2020
3e0de4d
only show the tooltip when the dropzone is unavaiable and visible
jloleysens Jun 25, 2020
a7fe179
keep white background on dim
jloleysens Jun 25, 2020
66cc71d
hide controls when moving
jloleysens Jun 25, 2020
b4ea683
fix on blur bug
jloleysens Jun 25, 2020
c38f0dd
Merge branch 'master' into ingest-pipelines/editor-dropzone-refinement
elasticmachine Jun 26, 2020
4c92d86
Rename variables and prefix booleans with "is"
jloleysens Jun 26, 2020
95f9a04
Remove box shadow on all nested tree items
jloleysens Jun 26, 2020
bf5338a
use classNames as it is intended to be used
jloleysens Jun 26, 2020
22c4288
Refactor SCSS values to variables
jloleysens Jun 26, 2020
7c0afb8
Added cancel move button
jloleysens Jun 26, 2020
8ffad8c
Fixes for monaco XJSON grammar parser and update form copy
jloleysens Jun 26, 2020
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
3 changes: 2 additions & 1 deletion packages/kbn-monaco/src/xjson/grammar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,13 @@ export const createParser = () => {

try {
value();
white();
} catch (e) {
errored = true;
annos.push({ type: AnnoTypes.error, at: e.at - 1, text: e.message });
}
if (!errored && ch) {
error('Syntax error');
annos.push({ type: AnnoTypes.error, at: at, text: 'Syntax Error' });
}
return { annotations: annos };
}
Expand Down
5 changes: 4 additions & 1 deletion packages/kbn-monaco/src/xjson/language.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ export const registerGrammarChecker = (editor: monaco.editor.IEditor) => {

const updateAnnos = async () => {
const { annotations } = await wps.getAnnos();
const model = editor.getModel() as monaco.editor.ITextModel;
const model = editor.getModel() as monaco.editor.ITextModel | null;
if (!model) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what are the scenarios in which there wouldn't be a model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is quite specific to how monaco works, and tbh I am not entirely certain 😅 . They do provide types that support this claim and I have seen that, on occasion, at runtime when editing the model is unavailable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining! Might be worth leaving a comment in the code with this info.

return;
}
monaco.editor.setModelMarkers(
model,
OWNER,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export const PipelineForm: React.FunctionComponent<PipelineFormProps> = ({
});

const onEditorFlyoutOpen = useCallback(() => {
setIsTestingPipeline(false);
setIsRequestVisible(false);
}, [setIsRequestVisible]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,15 @@ jest.mock('@elastic/eui', () => {
}}
/>
),
// Mocking EuiCodeEditor, which uses React Ace under the hood
EuiCodeEditor: (props: any) => (
};
});

jest.mock('../../../../../../../../src/plugins/kibana_react/public', () => {
const original = jest.requireActual('../../../../../../../../src/plugins/kibana_react/public');
return {
...original,
// Mocking CodeEditor, which uses React Monaco under the hood
CodeEditor: (props: any) => (
<input
data-test-subj={props['data-test-subj'] || 'mockCodeEditor'}
data-currentvalue={props.value}
Expand Down Expand Up @@ -95,8 +102,9 @@ const createActions = (testBed: TestBed<TestSubject>) => {
act(() => {
find(`${processorSelector}.moveItemButton`).simulate('click');
});
component.update();
act(() => {
find(dropZoneSelector).last().simulate('click');
find(dropZoneSelector).simulate('click');
});
component.update();
},
Expand All @@ -122,13 +130,6 @@ const createActions = (testBed: TestBed<TestSubject>) => {
});
},

duplicateProcessor(processorSelector: string) {
find(`${processorSelector}.moreMenu.button`).simulate('click');
act(() => {
find(`${processorSelector}.moreMenu.duplicateButton`).simulate('click');
});
},

startAndCancelMove(processorSelector: string) {
act(() => {
find(`${processorSelector}.moveItemButton`).simulate('click');
Expand All @@ -139,6 +140,13 @@ const createActions = (testBed: TestBed<TestSubject>) => {
});
},

duplicateProcessor(processorSelector: string) {
find(`${processorSelector}.moreMenu.button`).simulate('click');
act(() => {
find(`${processorSelector}.moreMenu.duplicateButton`).simulate('click');
});
},

toggleOnFailure() {
find('pipelineEditorOnFailureToggle').simulate('click');
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
$dropZoneZIndex: 1; /* Prevent the next item down from obscuring the button */
$cancelButtonZIndex: 2;
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const OnFailureProcessorsTitle: FunctionComponent = () => {
<EuiText size="s" color="subdued">
<FormattedMessage
id="xpack.ingestPipelines.pipelineEditor.onFailureTreeDescription"
defaultMessage="The processors used to pre-process documents before indexing. {learnMoreLink}"
defaultMessage="The processors used to handle exceptions in this pipeline. {learnMoreLink}"
values={{
learnMoreLink: (
<EuiLink
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import classNames from 'classnames';
import React, { FunctionComponent, useState } from 'react';

import { EuiContextMenuItem, EuiContextMenuPanel, EuiPopover, EuiButtonIcon } from '@elastic/eui';
Expand All @@ -12,6 +13,7 @@ import { editorItemMessages } from './messages';

interface Props {
disabled: boolean;
hidden: boolean;
showAddOnFailure: boolean;
onDuplicate: () => void;
onDelete: () => void;
Expand All @@ -20,9 +22,13 @@ interface Props {
}

export const ContextMenu: FunctionComponent<Props> = (props) => {
const { showAddOnFailure, onDuplicate, onAddOnFailure, onDelete, disabled } = props;
const { showAddOnFailure, onDuplicate, onAddOnFailure, onDelete, disabled, hidden } = props;
const [isOpen, setIsOpen] = useState<boolean>(false);

const containerClasses = classNames({
'pipelineProcessorsEditor__item--displayNone': hidden,
});

const contextMenuItems = [
<EuiContextMenuItem
data-test-subj="duplicateButton"
Expand Down Expand Up @@ -63,23 +69,25 @@ export const ContextMenu: FunctionComponent<Props> = (props) => {
].filter(Boolean) as JSX.Element[];

return (
<EuiPopover
data-test-subj={props['data-test-subj']}
anchorPosition="leftCenter"
panelPaddingSize="none"
isOpen={isOpen}
closePopover={() => setIsOpen(false)}
button={
<EuiButtonIcon
data-test-subj="button"
disabled={disabled}
onClick={() => setIsOpen((v) => !v)}
iconType="boxesHorizontal"
aria-label={editorItemMessages.moreButtonAriaLabel}
/>
}
>
<EuiContextMenuPanel items={contextMenuItems} />
</EuiPopover>
<div className={containerClasses}>
<EuiPopover
data-test-subj={props['data-test-subj']}
anchorPosition="leftCenter"
panelPaddingSize="none"
isOpen={isOpen}
closePopover={() => setIsOpen(false)}
button={
<EuiButtonIcon
data-test-subj="button"
disabled={disabled}
onClick={() => setIsOpen((v) => !v)}
iconType="boxesHorizontal"
aria-label={editorItemMessages.moreButtonAriaLabel}
/>
}
>
<EuiContextMenuPanel items={contextMenuItems} />
</EuiPopover>
</div>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,20 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import classNames from 'classnames';
import React, { FunctionComponent, useState, useEffect, useCallback } from 'react';
import { EuiFieldText, EuiText, keyCodes } from '@elastic/eui';

export interface Props {
placeholder: string;
ariaLabel: string;
onChange: (value: string) => void;
disabled: boolean;
text?: string;
}

export const InlineTextInput: FunctionComponent<Props> = ({
disabled,
placeholder,
text,
ariaLabel,
Expand All @@ -23,26 +25,17 @@ export const InlineTextInput: FunctionComponent<Props> = ({
const [isShowingTextInput, setIsShowingTextInput] = useState<boolean>(false);
const [textValue, setTextValue] = useState<string>(text ?? '');

const content = isShowingTextInput ? (
<EuiFieldText
controlOnly
fullWidth
compressed
value={textValue}
aria-label={ariaLabel}
className="pipelineProcessorsEditor__item__textInput"
inputRef={(el) => el?.focus()}
onChange={(event) => setTextValue(event.target.value)}
/>
) : (
<EuiText size="s" color="subdued">
{text || <em>{placeholder}</em>}
</EuiText>
);
const containerClasses = classNames('pipelineProcessorsEditor__item__textContainer', {
'pipelineProcessorsEditor__item__textContainer--notEditing': !isShowingTextInput && !disabled,
});

const submitChange = useCallback(() => {
setIsShowingTextInput(false);
onChange(textValue);
// Give any on blur handlers the chance to complete if the user is
// tabbing over this component.
setTimeout(() => {
setIsShowingTextInput(false);
onChange(textValue);
});
}, [setIsShowingTextInput, onChange, textValue]);

useEffect(() => {
Expand All @@ -62,14 +55,27 @@ export const InlineTextInput: FunctionComponent<Props> = ({
};
}, [isShowingTextInput, submitChange, setIsShowingTextInput]);

return (
<div
className="pipelineProcessorsEditor__item__textContainer"
tabIndex={0}
onFocus={() => setIsShowingTextInput(true)}
onBlur={submitChange}
>
{content}
return isShowingTextInput && !disabled ? (
<div className={`pipelineProcessorsEditor__item__textContainer ${containerClasses}`}>
<EuiFieldText
controlOnly
onBlur={submitChange}
fullWidth
compressed
value={textValue}
aria-label={ariaLabel}
className="pipelineProcessorsEditor__item__textInput"
inputRef={(el) => el?.focus()}
onChange={(event) => setTextValue(event.target.value)}
/>
</div>
) : (
<div className={containerClasses} tabIndex={0} onFocus={() => setIsShowingTextInput(true)}>
<EuiText size="s" color="subdued">
<div className="pipelineProcessorsEditor__item__description">
{text || <em>{placeholder}</em>}
</div>
</EuiText>
</div>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,9 @@ export const editorItemMessages = {
moveButtonLabel: i18n.translate('xpack.ingestPipelines.pipelineEditor.item.moveButtonLabel', {
defaultMessage: 'Move this processor',
}),
editorButtonLabel: i18n.translate(
'xpack.ingestPipelines.pipelineEditor.item.editButtonAriaLabel',
{
defaultMessage: 'Edit this processor',
}
),
editButtonLabel: i18n.translate('xpack.ingestPipelines.pipelineEditor.item.editButtonAriaLabel', {
defaultMessage: 'Edit this processor',
}),
duplicateButtonLabel: i18n.translate(
'xpack.ingestPipelines.pipelineEditor.item.moreMenu.duplicateButtonLabel',
{
Expand All @@ -31,7 +28,7 @@ export const editorItemMessages = {
cancelMoveButtonLabel: i18n.translate(
'xpack.ingestPipelines.pipelineEditor.item.cancelMoveButtonAriaLabel',
{
defaultMessage: 'Cancel moving this processor',
defaultMessage: 'Cancel move',
}
),
deleteButtonLabel: i18n.translate(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,57 @@
@import '../shared';

.pipelineProcessorsEditor__item {
transition: border-color 1s;
min-height: 50px;
&--selected {
border: 1px solid $euiColorPrimary;
}

&--displayNone {
display: none;
}

&--dimmed {
box-shadow: none;
}

// Remove the box-shadow on all nested items
.pipelineProcessorsEditor__item {
box-shadow: none !important;
}

&__processorTypeLabel {
line-height: $euiButtonHeightSmall;
}

&__textContainer {
padding: 4px;
border-radius: 2px;

transition: border-color .3s;
border: 2px solid #FFF;
transition: border-color 0.3s;
border: 2px solid transparent;

&:hover {
border: 2px solid $euiColorLightShade;
&--notEditing {
&:hover {
border: 2px solid $euiColorLightShade;
}
}
}

&__description {
overflow-x: hidden;
white-space: nowrap;
text-overflow: ellipsis;
max-width: 600px;
}

&__textInput {
height: 21px;
min-width: 100px;
min-width: 150px;
}

&__cancelMoveButton {
// Ensure that the cancel button is above the drop zones
z-index: $cancelButtonZIndex;
}
}
Loading