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

Change Role Mappings editor to use CodeEditor #107459

Merged
merged 3 commits into from
Aug 9, 2021
Merged

Change Role Mappings editor to use CodeEditor #107459

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3108d71
Change Role Mappings editor to use CodeEditor
jportner Aug 2, 2021
4ea4178
Merge branch 'master' into issue-107078-role-mappings-json-editor
kibanamachine Aug 9, 2021
6163ff0
Use CodeEditorField instead of CodeEditor
jportner Aug 9, 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
41 changes: 25 additions & 16 deletions ...ic/management/role_mappings/edit_role_mapping/rule_editor_panel/json_rule_editor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,27 @@ import 'brace/mode/json';
// warnings in the console which adds unnecessary noise to the test output.
import '@kbn/test/target_node/jest/utils/stub_web_worker';

import { EuiCodeEditor } from '@elastic/eui';
import React from 'react';
import { act } from 'react-dom/test-utils';

import { mountWithIntl } from '@kbn/test/jest';
import { coreMock } from 'src/core/public/mocks';
import { KibanaContextProvider } from 'src/plugins/kibana_react/public';
import type { monaco } from '@kbn/monaco';
import { shallowWithIntl } from '@kbn/test/jest';
import { CodeEditor } from 'src/plugins/kibana_react/public';
Copy link
Contributor

@thomheymann thomheymann Aug 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Have you tried using CodeEditorField component? It renders the code editor like a form field with the correct border and padding so it looks a lot nicer than the plain Monaco editor.

Example here: x-pack/plugins/security/public/management/api_keys/api_keys_grid/create_api_key_flyout.tsx

Copy link
Contributor Author

@jportner jportner Aug 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nice idea, I'll change it!

Edit: done in 6163ff0

Side by side comparison before/after
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, looking much better! :)


import { AllRule, AnyRule, ExceptAllRule, ExceptAnyRule, FieldRule } from '../../model';
import { JSONRuleEditor } from './json_rule_editor';

jest.mock('../../../../../../../../src/plugins/kibana_react/public', () => ({
...jest.requireActual('../../../../../../../../src/plugins/kibana_react/public'),
useKibana: jest.fn().mockReturnValue({
services: { docLinks: { links: { apis: { createRoleMapping: 'createRoleMappingLink' } } } },
}),
}));

describe('JSONRuleEditor', () => {
const mockChangeEvent = {} as monaco.editor.IModelContentChangedEvent;
const renderView = (props: React.ComponentProps<typeof JSONRuleEditor>) => {
const coreStart = coreMock.createStart();
return mountWithIntl(
<KibanaContextProvider services={coreStart}>
<JSONRuleEditor {...props} />
</KibanaContextProvider>
);
return shallowWithIntl(<JSONRuleEditor {...props} />);
};
Comment on lines +25 to 36
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest uses jsdom to render components, but Monaco requires a canvas. Instead of installing a separate package just to use for this test, I opted for a shallow render which still allows us to exercise these unit test cases.

Note that to use a shallow mock this has to be the top level component, so as a result I had to mock useKibana().


it('renders an empty rule set', () => {
Expand All @@ -40,7 +42,8 @@ describe('JSONRuleEditor', () => {
expect(props.onChange).not.toHaveBeenCalled();
expect(props.onValidityChange).not.toHaveBeenCalled();

expect(wrapper.find(EuiCodeEditor).props().value).toMatchInlineSnapshot(`"{}"`);
wrapper.update();
expect(wrapper.find(CodeEditor).props().value).toMatchInlineSnapshot(`"{}"`);
});

it('renders a rule set', () => {
Expand All @@ -58,7 +61,7 @@ describe('JSONRuleEditor', () => {
};
const wrapper = renderView(props);

const { value } = wrapper.find(EuiCodeEditor).props();
const { value } = wrapper.find(CodeEditor).props();
expect(JSON.parse(value as string)).toEqual({
all: [
{
Expand Down Expand Up @@ -89,7 +92,10 @@ describe('JSONRuleEditor', () => {

const allRule = JSON.stringify(new AllRule().toRaw());
act(() => {
wrapper.find(EuiCodeEditor).props().onChange!(allRule + ', this makes invalid JSON');
wrapper.find(CodeEditor).props().onChange!(
allRule + ', this makes invalid JSON',
mockChangeEvent
);
});

expect(props.onValidityChange).toHaveBeenCalledTimes(1);
Expand All @@ -112,7 +118,7 @@ describe('JSONRuleEditor', () => {
});

act(() => {
wrapper.find(EuiCodeEditor).props().onChange!(invalidRule);
wrapper.find(CodeEditor).props().onChange!(invalidRule, mockChangeEvent);
});

expect(props.onValidityChange).toHaveBeenCalledTimes(1);
Expand All @@ -126,7 +132,10 @@ describe('JSONRuleEditor', () => {

const allRule = JSON.stringify(new AllRule().toRaw());
act(() => {
wrapper.find(EuiCodeEditor).props().onChange!(allRule + ', this makes invalid JSON');
wrapper.find(CodeEditor).props().onChange!(
allRule + ', this makes invalid JSON',
mockChangeEvent
);
});

expect(props.onValidityChange).toHaveBeenCalledTimes(1);
Expand All @@ -136,7 +145,7 @@ describe('JSONRuleEditor', () => {
props.onValidityChange.mockReset();

act(() => {
wrapper.find(EuiCodeEditor).props().onChange!(allRule);
wrapper.find(CodeEditor).props().onChange!(allRule, mockChangeEvent);
});

expect(props.onValidityChange).toHaveBeenCalledTimes(1);
Expand Down
34 changes: 18 additions & 16 deletions .../public/management/role_mappings/edit_role_mapping/rule_editor_panel/json_rule_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@
import 'brace/mode/json';
import 'brace/theme/github';

import { EuiButton, EuiCodeEditor, EuiFormRow, EuiLink, EuiSpacer, EuiText } from '@elastic/eui';
import { EuiButton, EuiFormRow, EuiLink, EuiSpacer, EuiText } from '@elastic/eui';
import React, { Fragment, useState } from 'react';

import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import { XJsonLang } from '@kbn/monaco';

import { useKibana } from '../../../../../../../../src/plugins/kibana_react/public';
import { CodeEditor, useKibana } from '../../../../../../../../src/plugins/kibana_react/public';
import type { Rule } from '../../model';
import { generateRulesFromRaw, RuleBuilderError } from '../../model';

Expand Down Expand Up @@ -76,25 +77,26 @@ export const JSONRuleEditor = (props: Props) => {
data-test-subj="roleMappingsJSONEditor"
>
<Fragment>
<EuiCodeEditor
<CodeEditor
aria-label={''}
mode={'json'}
theme="github"
languageId={XJsonLang.ID}
value={rawRules}
onChange={onRulesChange}
width="100%"
height="auto"
minLines={6}
maxLines={30}
isReadOnly={false}
setOptions={{
showLineNumbers: true,
fullWidth={true}
height="300px"
Comment on lines -87 to +89
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 component doesn't support maxLines/minLines but it needs a height, so I just set it to 300px which seems reasonable (which equals around 17 lines), right in the middle).

options={{
accessibilitySupport: 'off',
Copy link
Contributor Author

@jportner jportner Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Disabling accessibilitySupport sounds bad, but I think it's necessary. This prevents Monaco from enabling shortcuts such as these that appear to be selected by default on macOS:

image

None of these should be used when editing code. It just causes frustration when typing.

Seeing as this is also an optional UI for editing role mappings, I think it's safe to disable accessibilitySupport here.

lineNumbers: 'on',
fontSize: 12,
tabSize: 2,
automaticLayout: true,
minimap: { enabled: false },
overviewRulerBorder: false,
scrollbar: { alwaysConsumeMouseWheel: false },
scrollBeyondLastLine: false,
wordWrap: 'on',
wrappingIndent: 'indent',
}}
editorProps={{
$blockScrolling: Infinity,
}}
showGutter={true}
/>
<EuiSpacer size="s" />
<EuiButton iconType="broom" onClick={reformatRules} size="s">
Expand Down