[Mappings editor] Add json editor to edit field settings#47674
[Mappings editor] Add json editor to edit field settings#47674sebelga merged 4 commits intoelastic:feature/mappings-editorfrom
Conversation
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
|
||
| export type OnUpdateHandler<T = { [key: string]: any }> = (arg: { | ||
| getData(): T; | ||
| data: { |
There was a problem hiding this comment.
@jloleysens I slightly changed the contract to get the json data in order to align with the contract of the form hook update.
💚 Build Succeeded |
jloleysens
left a comment
There was a problem hiding this comment.
Did not test this locally, but overall this looks good!
I left some comments you can choose to address or not.
| export const MappingsEditor = React.memo(({ onUpdate, defaultValue }: Props) => { | ||
| const configurationDefaultValue = useMemo( | ||
| () => | ||
| defaultValue === undefined |
There was a problem hiding this comment.
Hmm, I think this filter + reduce pattern can be made slightly less verbose:
// something like
useMemo(() => _.pick(defaultValue === undefined ? {} : defaultValue, CONFIGURATION_FIELDS), [defaultValue]);Did not test, may need to jam a Types['MappingsConfiguration'] in somewhere 😄
There was a problem hiding this comment.
Great idea, I haven't used pick in the past but indeed it makes it much easier to read. I'll merge the PR and update that part of the code. 👍
| ...acc, | ||
| [key]: value, | ||
| }), | ||
| {} as Types['MappingsConfiguration'] |
There was a problem hiding this comment.
I'm also wondering if this namespacing of interfaces under Types helps. As a reader I find this quite a verbose way of specifying the interface we're targeting. If we want namespacing I'd prefer this pattern (using file as a namespace provider):
import * as types from './my_types'
...
const a: types.MappingsConfiguration = {};Just my 2 cents :)
There was a problem hiding this comment.
I take note and will look how the exported types could be exported differently. It's mainly to avoid name conflict exporting interfaces and components from the same index.ts file.
But I am not sure that types.MappingsConfiguration is much less verbose than Types['MappingsConfiguration']. 😊
|
Thanks for the review @jloleysens ! I'll update using |
This PR adds a JSON editor to edit a field setting in the flyout.