-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Mappings Editor] JSON Editor #47589
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
Changes from all commits
dbe180b
21c8a72
c8f4774
56ae16b
5dd9ec3
2c92e55
fb4a423
3c7e33e
3beedf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import React, { useRef, useCallback } from 'react'; | ||
|
|
||
| import { useDispatch } from '../../mappings_state'; | ||
| import { JsonEditor } from '../../../json_editor'; | ||
|
|
||
| export interface Props { | ||
| defaultValue: object; | ||
| } | ||
|
|
||
| export const DocumentFieldsJsonEditor = ({ defaultValue }: Props) => { | ||
| const dispatch = useDispatch(); | ||
| const defaultValueRef = useRef(defaultValue); | ||
| const onUpdate = useCallback( | ||
| ({ data, isValid }) => | ||
| dispatch({ | ||
| type: 'fieldsJsonEditor.update', | ||
| value: { json: data.format(), isValid }, | ||
| }), | ||
| [dispatch] | ||
| ); | ||
| return <JsonEditor onUpdate={onUpdate} defaultValue={defaultValueRef.current} />; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import React from 'react'; | ||
| import { EuiButton, EuiText } from '@elastic/eui'; | ||
|
|
||
| import { useDispatch, useState } from '../../mappings_state'; | ||
| import { FieldsEditor } from '../../types'; | ||
| import { canUseMappingsEditor, normalize } from '../../lib'; | ||
|
|
||
| interface Props { | ||
| editor: FieldsEditor; | ||
| } | ||
|
|
||
| /* TODO: Review toggle controls UI */ | ||
| export const EditorToggleControls = ({ editor }: Props) => { | ||
| const dispatch = useDispatch(); | ||
| const { fieldsJsonEditor } = useState(); | ||
|
|
||
| const [showMaxDepthWarning, setShowMaxDepthWarning] = React.useState<boolean>(false); | ||
| const [showValidityWarning, setShowValidityWarning] = React.useState<boolean>(false); | ||
|
|
||
| const clearWarnings = () => { | ||
| if (showMaxDepthWarning) { | ||
| setShowMaxDepthWarning(false); | ||
| } | ||
|
|
||
| if (showValidityWarning) { | ||
| setShowValidityWarning(false); | ||
| } | ||
| }; | ||
|
|
||
| if (editor === 'default') { | ||
| clearWarnings(); | ||
| return ( | ||
| <EuiButton | ||
| onClick={() => { | ||
| dispatch({ type: 'documentField.changeEditor', value: 'json' }); | ||
| }} | ||
| > | ||
| Use JSON Editor | ||
| </EuiButton> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <EuiButton | ||
| onClick={() => { | ||
| clearWarnings(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to have an external Also, if the clearWarnings();
const { isValid } = state.fieldsJsonEditor;
if (!isValid) {
setShowValidityWarning(true);
} else {
const deNormalizedFields = state.fieldsJsonEditor.format();
const { maxNestedDepth } = normalize(deNormalizedFields);
const canUseDefaultEditor = canUseMappingsEditor(maxNestedDepth);
if (canUseDefaultEditor) {
dispatch({ type: 'documentField.changeEditor', value: 'default' });
} else {
setShowMaxDepthWarning(true);
}
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure - I don't have a preference either way here regarding in-lining so we can go with making it external. +1 for not normalizing if the JSON is invalid.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, I think I see better what you meant regarding inlining. This maybe comes down more to personal preference for me, but I think this reads a bit better since the one handler is so small. |
||
| const { isValid } = fieldsJsonEditor; | ||
| if (!isValid) { | ||
| setShowValidityWarning(true); | ||
| } else { | ||
| const deNormalizedFields = fieldsJsonEditor.format(); | ||
| const { maxNestedDepth } = normalize(deNormalizedFields); | ||
| const canUseDefaultEditor = canUseMappingsEditor(maxNestedDepth); | ||
|
|
||
| if (canUseDefaultEditor) { | ||
| dispatch({ type: 'documentField.changeEditor', value: 'default' }); | ||
| } else { | ||
| setShowMaxDepthWarning(true); | ||
| } | ||
| } | ||
| }} | ||
| > | ||
| Use Mappings Editor | ||
| </EuiButton> | ||
| {showMaxDepthWarning ? ( | ||
| <EuiText size="s" color="danger"> | ||
| Max depth for Mappings Editor exceeded | ||
| </EuiText> | ||
| ) : null} | ||
| {showValidityWarning && !fieldsJsonEditor.isValid ? ( | ||
| <EuiText size="s" color="danger"> | ||
| JSON is invalid | ||
| </EuiText> | ||
| ) : null} | ||
| </> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| jest.mock('../constants', () => ({ DATA_TYPE_DEFINITION: {} })); | ||
|
|
||
| import { determineIfValid } from '.'; | ||
|
|
||
| describe('Mappings Editor form validity', () => { | ||
| let components: any; | ||
| it('handles base case', () => { | ||
| components = { | ||
| fieldsJsonEditor: { isValid: undefined }, | ||
| configuration: { isValid: undefined }, | ||
| fieldForm: undefined, | ||
| }; | ||
| expect(determineIfValid(components)).toBe(undefined); | ||
| }); | ||
|
|
||
| it('handles combinations of true, false and undefined', () => { | ||
| components = { | ||
| fieldsJsonEditor: { isValid: false }, | ||
| configuration: { isValid: true }, | ||
| fieldForm: undefined, | ||
| }; | ||
|
|
||
| expect(determineIfValid(components)).toBe(false); | ||
|
|
||
| components = { | ||
| fieldsJsonEditor: { isValid: false }, | ||
| configuration: { isValid: undefined }, | ||
| fieldForm: undefined, | ||
| }; | ||
|
|
||
| expect(determineIfValid(components)).toBe(undefined); | ||
|
|
||
| components = { | ||
| fieldsJsonEditor: { isValid: true }, | ||
| configuration: { isValid: undefined }, | ||
| fieldForm: undefined, | ||
| }; | ||
|
|
||
| expect(determineIfValid(components)).toBe(undefined); | ||
|
|
||
| components = { | ||
| fieldsJsonEditor: { isValid: true }, | ||
| configuration: { isValid: false }, | ||
| fieldForm: undefined, | ||
| }; | ||
|
|
||
| expect(determineIfValid(components)).toBe(false); | ||
|
|
||
| components = { | ||
| fieldsJsonEditor: { isValid: false }, | ||
| configuration: { isValid: true }, | ||
| fieldForm: { isValid: true }, | ||
| }; | ||
|
|
||
| expect(determineIfValid(components)).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,8 @@ import { | |
| SubType, | ||
| ChildFieldName, | ||
| } from '../types'; | ||
| import { DATA_TYPE_DEFINITION } from '../constants'; | ||
| import { DATA_TYPE_DEFINITION, MAX_DEPTH_DEFAULT_EDITOR } from '../constants'; | ||
| import { State } from '../reducer'; | ||
|
|
||
| export const getUniqueId = () => { | ||
| return ( | ||
|
|
@@ -243,3 +244,27 @@ export const shouldDeleteChildFieldsAfterTypeChange = ( | |
|
|
||
| return false; | ||
| }; | ||
|
|
||
| export const canUseMappingsEditor = (maxNestedDepth: number) => | ||
| maxNestedDepth < MAX_DEPTH_DEFAULT_EDITOR; | ||
|
|
||
| const stateWithValidity: Array<keyof State> = ['configuration', 'fieldsJsonEditor', 'fieldForm']; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, the one drawback I can see with this is that we now have two places that need to be kept in sync for validation to function correctly. I think if it's documented then it's totally fine to take this approach. One alternative could be that these entries are placed under a key like |
||
|
|
||
| export const determineIfValid = (state: State): boolean | undefined => | ||
| Object.entries(state) | ||
| .filter(([key]) => stateWithValidity.includes(key as keyof State)) | ||
| .reduce( | ||
| (isValid, { 1: value }) => { | ||
| if (value === undefined) { | ||
| return isValid; | ||
| } | ||
|
|
||
| // If one section validity of the state is "undefined", the mappings validity is also "undefined" | ||
| if (isValid === undefined || value.isValid === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return isValid && value.isValid; | ||
| }, | ||
| true as undefined | boolean | ||
| ); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need this ref, can't we directly use the props provided? And maybe wrap the component with
react.memo?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this instance
useRefis done to only take the firstdefaultValuevalue that comes in and not respond to changes to defaultValue thereafter - which is differentreact.memowould do I think? I think ofdefaultValueactually more asinitialValue. If that makes sense?