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

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

merged 3 commits into from
Aug 9, 2021

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Aug 2, 2021 •
edited
Loading

Resolves #107078.

Before:
image

After:
image

Manually tested various bits of functionality (error detection, syntax highlighting, scrolling, resizing, etc. All works fine with the new editor. Used src/plugins/discover/public/application/components/json_code_editor/json_code_editor_common.tsx for inspiration.

@jportner jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Aug 2, 2021
jportner
jportner commented Aug 2, 2021
View reviewed changes
Copy link
Contributor Author

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Author's notes for reviewers

...ity/public/management/role_mappings/edit_role_mapping/rule_editor_panel/json_rule_editor.tsx
Comment on lines -87 to +86
minLines={6}
maxLines={30}
isReadOnly={false}
setOptions={{
showLineNumbers: true,
fullWidth={true}
height="300px"
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).

...ity/public/management/role_mappings/edit_role_mapping/rule_editor_panel/json_rule_editor.tsx
fullWidth={true}
height="300px"
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.

...ublic/management/role_mappings/edit_role_mapping/rule_editor_panel/json_rule_editor.test.tsx
Comment on lines +25 to 36
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} />);
};
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().

@legrego legrego marked this pull request as ready for review August 3, 2021 13:21
@legrego legrego requested a review from a team as a code owner August 3, 2021 13:21
@jportner
Copy link
Contributor Author

jportner commented Aug 9, 2021

@elasticmachine merge upstream

@jportner jportner requested a review from thomheymann August 9, 2021 16:02
thomheymann
thomheymann reviewed Aug 9, 2021
View reviewed changes
Copy link
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

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

Looks good, one question below.

...ublic/management/role_mappings/edit_role_mapping/rule_editor_panel/json_rule_editor.test.tsx Outdated
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! :)

@jportner jportner requested a review from thomheymann August 9, 2021 16:47
thomheymann
thomheymann approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

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

LGTM!

@jportner jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 9, 2021
@jportner jportner enabled auto-merge (squash) August 9, 2021 17:33
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
security 775.1KB 775.3KB +180.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
security 84.9KB 84.9KB +69.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jportner jportner merged commit 475c618 into elastic:master Aug 9, 2021
@jportner jportner deleted the issue-107078-role-mappings-json-editor branch August 9, 2021 19:10
@kibanamachine kibanamachine mentioned this pull request Aug 9, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 9, 2021
@jportner @kibanamachine
Change Role Mappings editor to use CodeEditor (elastic#107459)
7bc8699
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 11, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Aug 11, 2021
@kibanamachine @jportner
Change Role Mappings editor to use CodeEditor (#107459) (#107958)
0a65977 
Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 11, 2021
@jportner jportner mentioned this pull request Aug 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomheymann thomheymann thomheymann approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate role mappings JSON editor to use the monaco-based editor
3 participants
@jportner @kibanamachine @thomheymann