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

[Discover] Add Footer Bar for Single Line Editor #8565

Merged
merged 9 commits into from
Oct 23, 2024
Merged
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8565.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Adds editor footer to single line editor on focus ([#8565](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8565))
2 changes: 1 addition & 1 deletion src/plugins/data/common/data_frames/types.ts
Original file line number Diff line number Diff line change
@@ -121,7 +121,7 @@ export type IDataFrameResponse = SearchResponse<any> &
(IDataFrameDefaultResponse | IDataFramePollingResponse | IDataFrameErrorResponse);

export interface IDataFrameError extends SearchResponse<any> {
error: Error;
error: Error | string;
}

export interface PollQueryResultsParams {
7 changes: 1 addition & 6 deletions src/plugins/data/common/utils/helpers.ts
Original file line number Diff line number Diff line change
@@ -28,7 +28,6 @@
* under the License.
*/

import { i18n } from '@osd/i18n';
import { PollQueryResultsHandler, FetchStatusResponse } from '../data_frames';

export interface QueryStatusOptions {
@@ -53,11 +52,7 @@
} while (queryStatus !== 'SUCCESS' && queryStatus !== 'FAILED');

if (queryStatus === 'FAILED') {
throw new Error(
i18n.translate('data.search.request.failed', {
defaultMessage: 'An error occurred while executing the search query',
})
);
throw new Error(queryResultsRes?.body.error);

Check warning on line 55 in src/plugins/data/common/utils/helpers.ts

Codecov / codecov/patch

src/plugins/data/common/utils/helpers.ts#L55

Added line #L55 was not covered by tests
}

return queryResultsRes;

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

Original file line number Diff line number Diff line change
@@ -7,3 +7,8 @@
background-color: $euiColorLightestShade;
}
}

.editor_footerItem {
// Needed so the footer items never have paddings
padding: 0 !important;
}
Original file line number Diff line number Diff line change
@@ -22,10 +22,9 @@
status: ResultStatus;
body?: {
error?: {
reason?: string;
details: string;
statusCode?: number;
Copy link
Member

Choose a reason for hiding this comment

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

why do we have this here? We dont want to show the user the status code, but rather the error and the reason

message?: string;
};
statusCode?: number;
};
elapsedMs?: number;
startTime?: number;
@@ -77,6 +76,22 @@
</EuiButtonEmpty>
);
}
const time = Math.floor(elapsedTime / 1000);
return (

Check warning on line 80 in src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx

Codecov / codecov/patch

src/plugins/data/public/query/query_string/language_service/lib/query_result.tsx#L79-L80

Added lines #L79 - L80 were not covered by tests
<EuiButtonEmpty
color="text"
size="xs"
onClick={() => {}}
isLoading
data-test-subj="queryResultLoading"
className="editor__footerItem"
>
{i18n.translate('data.query.languageService.queryResults.loadTime', {
defaultMessage: 'Loading {time} s',
values: { time },
})}
</EuiButtonEmpty>
);
}

if (props.queryStatus.status === ResultStatus.READY) {
@@ -101,7 +116,13 @@
}

return (
<EuiButtonEmpty iconSide="left" iconType={'checkInCircleEmpty'} size="xs" onClick={() => {}}>
<EuiButtonEmpty
iconSide="left"
iconType={'checkInCircleEmpty'}
iconGap="s"
size="xs"
onClick={() => {}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we have a button that does nothing? is this the right ui element?

>
<EuiText size="xs" color="subdued" data-test-subj="queryResultCompleteMsg">
{message}
</EuiText>
@@ -122,8 +143,10 @@
size="xs"
onClick={onButtonClick}
data-test-subj="queryResultErrorBtn"
className="editor__footerItem"
color="danger"
>
<EuiText size="xs" color="subdued">
<EuiText size="xs" color="danger" className="editor__footerItem">
{i18n.translate('data.query.languageService.queryResults.error', {
defaultMessage: `Error`,
})}
@@ -137,23 +160,15 @@
data-test-subj="queryResultError"
>
<EuiPopoverTitle>ERRORS</EuiPopoverTitle>
<div style={{ width: '250px' }}>
<EuiText size="s">
<strong>
{i18n.translate('data.query.languageService.queryResults.reasons', {
defaultMessage: `Reasons:`,
})}
</strong>
{props.queryStatus.body.error.reason}
</EuiText>
<div style={{ width: '250px' }} className="eui-textBreakWord">
<EuiText size="s">
<p>
<strong>
{i18n.translate('data.query.languageService.queryResults.details', {
defaultMessage: `Details:`,
{i18n.translate('data.query.languageService.queryResults.message', {
defaultMessage: `Message:`,
})}
</strong>
{props.queryStatus.body.error.details}
</strong>{' '}
{props.queryStatus.body.error.message}
</p>
</EuiText>
</div>
37 changes: 37 additions & 0 deletions src/plugins/data/public/ui/query_editor/_query_editor.scss
Original file line number Diff line number Diff line change
@@ -218,3 +218,40 @@
display: block;
}
}

.queryEditor__footer {
Copy link
Collaborator

@LDrago27 LDrago27 Oct 16, 2024

Choose a reason for hiding this comment

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

For the values below can we use the constants from oui like $ouiSizeM and others.

Copy link
Member

Choose a reason for hiding this comment

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

+1. do this where it makes sense. I think the 5px values are needed to match a specific spacing issue so that could be ignored, but Gap could be an oui value

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

@sejli lets fix this in a fast follow

Copy link
Collaborator

Choose a reason for hiding this comment

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

OUI's flexboxes are messed up; they simulate gap by adding margins on children and negative margins on parent and this occasionally brings in unpleasant side effects.

display: flex;
gap: 4px;
background: $euiColorLightestShade;
padding: 2px 4px;
margin-top: 5px;
margin-left: -5px;
margin-right: -5px;
z-index: 1;
position: relative;
align-items: center;
}

.queryEditor__footerSpacer {
flex-grow: 1;
}

.queryEditor__footerItem {
// Needed so the footer items never have paddings
padding: 0 !important;
}

// TODO: Temporary workaround to disable padding for single line editor footer
Copy link
Member

Choose a reason for hiding this comment

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

Can we have an issue for this with the label tech debt?

.euiFormControlLayout--group.euiFormControlLayout--compressed .osdQuerEditor__singleLine .euiText {
padding-top: 0 !important;
padding-bottom: 0 !important;
}

.euiFormControlLayout--group .osdQuerEditor__singleLine .euiText {
background-color: unset !important;
line-height: 21px !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we do something custom to override default behavior, can we include a comment with the rationale?

Copy link
Member

Choose a reason for hiding this comment

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

Discover as a whole has a lot of overrides. This was an accepted deviation given the timelines. we will need to discuss how much of this needs to move to OUI vs stay custom long term

}

.euiFormControlLayout--group .osdQuerEditor__singleLine .euiButtonEmpty {
border-right: 0;
}
161 changes: 106 additions & 55 deletions src/plugins/data/public/ui/query_editor/editors/shared.tsx
Original file line number Diff line number Diff line change
@@ -3,9 +3,9 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { EuiCompressedFieldText } from '@elastic/eui';
import { EuiCompressedFieldText, EuiFlexGroup, EuiFlexItem } from '@elastic/eui';
import { monaco } from '@osd/monaco';
import React from 'react';
import React, { Fragment, useCallback, useRef, useState } from 'react';
import { CodeEditor } from '../../../../../opensearch_dashboards_react/public';

interface SingleLineInputProps extends React.JSX.IntrinsicAttributes {
@@ -15,6 +15,7 @@
editorDidMount: (editor: any) => void;
provideCompletionItems: monaco.languages.CompletionItemProvider['provideCompletionItems'];
prepend?: React.ComponentProps<typeof EuiCompressedFieldText>['prepend'];
footerItems?: any;
}

type CollapsedComponent<T> = React.ComponentType<T>;
@@ -61,59 +62,109 @@
editorDidMount,
provideCompletionItems,
prepend,
}) => (
<div className="euiFormControlLayout euiFormControlLayout--compressed euiFormControlLayout--group osdQueryBar__wrap">
{prepend}
<div
className="osdQuerEditor__singleLine euiFormControlLayout__childrenWrapper"
data-test-subj="osdQueryEditor__singleLine"
>
<CodeEditor
height={20} // Adjusted to match lineHeight for a single line
languageId={languageId}
value={value}
onChange={onChange}
editorDidMount={editorDidMount}
options={{
lineNumbers: 'off', // Disabled line numbers
// lineHeight: 40,
fontSize: 14,
fontFamily: 'Roboto Mono',
minimap: {
enabled: false,
},
scrollBeyondLastLine: false,
wordWrap: 'off', // Disabled word wrapping
wrappingIndent: 'none', // No indent since wrapping is off
folding: false,
glyphMargin: false,
lineDecorationsWidth: 0,
scrollbar: {
vertical: 'hidden',
},
overviewRulerLanes: 0,
hideCursorInOverviewRuler: true,
cursorStyle: 'line',
wordBasedSuggestions: false,
}}
suggestionProvider={{
provideCompletionItems,
triggerCharacters: [' '],
}}
languageConfiguration={{
autoClosingPairs: [
{
open: '(',
close: ')',
footerItems,
}) => {
const [editorIsFocused, setEditorIsFocused] = useState(false);
const blurTimeoutRef = useRef<NodeJS.Timeout | undefined>();

Check warning on line 68 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L67-L68

Added lines #L67 - L68 were not covered by tests

const handleEditorDidMount = useCallback(

Check warning on line 70 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L70

Added line #L70 was not covered by tests
(editor: monaco.editor.IStandaloneCodeEditor) => {
editorDidMount(editor);

Check warning on line 72 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L72

Added line #L72 was not covered by tests

const focusDisposable = editor.onDidFocusEditorText(() => {

Check warning on line 74 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L74

Added line #L74 was not covered by tests
if (blurTimeoutRef.current) {
clearTimeout(blurTimeoutRef.current);

Check warning on line 76 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L76

Added line #L76 was not covered by tests
}
setEditorIsFocused(true);

Check warning on line 78 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L78

Added line #L78 was not covered by tests
});

const blurDisposable = editor.onDidBlurEditorText(() => {
blurTimeoutRef.current = setTimeout(() => {
setEditorIsFocused(false);

Check warning on line 83 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L81-L83

Added lines #L81 - L83 were not covered by tests
}, 500);
});

return () => {
focusDisposable.dispose();
blurDisposable.dispose();

Check warning on line 89 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L87-L89

Added lines #L87 - L89 were not covered by tests
if (blurTimeoutRef.current) {
clearTimeout(blurTimeoutRef.current);

Check warning on line 91 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L91

Added line #L91 was not covered by tests
}
};
},
[editorDidMount]
);

return (

Check warning on line 98 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L98

Added line #L98 was not covered by tests
<div className="euiFormControlLayout euiFormControlLayout--compressed euiFormControlLayout--group osdQueryBar__wrap">
Copy link
Collaborator

Choose a reason for hiding this comment

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

qq: is there a reason to use the classes instead of components?

{prepend}
<div
className="osdQuerEditor__singleLine euiFormControlLayout__childrenWrapper"
data-test-subj="osdQueryEditor__singleLine"
>
<CodeEditor
height={20} // Adjusted to match lineHeight for a single line
languageId={languageId}
value={value}
onChange={onChange}
editorDidMount={handleEditorDidMount}
options={{
lineNumbers: 'off', // Disabled line numbers
// lineHeight: 40,
fontSize: 14,
fontFamily: 'Roboto Mono',
minimap: {
enabled: false,
},
{
open: '"',
close: '"',
scrollBeyondLastLine: false,
wordWrap: 'off', // Disabled word wrapping
wrappingIndent: 'none', // No indent since wrapping is off
folding: false,
glyphMargin: false,
lineDecorationsWidth: 0,
scrollbar: {
vertical: 'hidden',
sejli marked this conversation as resolved.
Show resolved Hide resolved
horizontalScrollbarSize: 1,
},
],
}}
triggerSuggestOnFocus={true}
/>
overviewRulerLanes: 0,
hideCursorInOverviewRuler: true,
cursorStyle: 'line',
wordBasedSuggestions: false,
}}
suggestionProvider={{
provideCompletionItems,
triggerCharacters: [' '],
}}
languageConfiguration={{
autoClosingPairs: [
{
open: '(',
close: ')',
},
{
open: '"',
close: '"',
},
],
}}
triggerSuggestOnFocus={true}
/>
{editorIsFocused && (
<div className="queryEditor__footer">
{footerItems && (
<Fragment>
{footerItems.start?.map((item) => (
<div className="queryEditor__footerItem">{item}</div>

Check warning on line 157 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L157

Added line #L157 was not covered by tests
))}
<div className="queryEditor__footerSpacer" />
{footerItems.end?.map((item) => (
<div className="queryEditor__footerItem">{item}</div>

Check warning on line 161 in src/plugins/data/public/ui/query_editor/editors/shared.tsx

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/editors/shared.tsx#L161

Added line #L161 was not covered by tests
))}
</Fragment>
)}
</div>
)}
</div>
</div>
</div>
);
);
};
Loading