-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Ingest Pipelines] Add descriptions for ingest processors A-D #75975
[Ingest Pipelines] Add descriptions for ingest processors A-D #75975
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
Great work @lockewritesdocs ! This PR is looking in good shape, thanks for making those copy changes!
I left a few comments in the code that would be great if you could take a look at. It also looks like we are missing type annotations here for the new field (the thing I would like to block on):
Line 36 in c233ecf
interface FieldDescriptor { |
I see you have named the new field helpText
which aligns well with the name in the EuiFormRow which we are going to be using, but I think description
would be a better name for this text.
@@ -68,13 +68,15 @@ export const ProcessorTypeField: FunctionComponent<Props> = ({ initialType }) => | |||
<UseField<string> config={typeConfig} defaultValue={initialType} path="type"> | |||
{(typeField) => { | |||
let selectedOptions: ProcessorTypeAndLabel[]; | |||
let helpText: string = ''; |
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.
It looks like we still need to assign this value to the EuiFormRow below
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.
You can also drop the : string
annotation. If you init this value to a string TS will figure out that it is a string automatically.
if (typeField.value?.length) { | ||
const type = typeField.value; | ||
const descriptor = getProcessorDescriptor(type); | ||
selectedOptions = descriptor | ||
? [{ label: descriptor.label, value: type }] | ||
: // If there is no label for this processor type, just use the type as the label | ||
[{ label: type, value: type }]; | ||
helpText = descriptor?.helpText || ''; |
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.
We could probably restructure this code now to something like:
const procesorDescriptor = getProcessorDescriptor(type);
if (procesorDescriptor) {
selectedOptions = [{ label: procesorDescriptor.label, value: type }];
helpText = procesorDescriptor.description; // assuming we are using the key `description`
} else {
// If there is no label for this processor type, just use the type as the label
selectedOptions = [{ label: type, value: type }];
}
I think this would read a bit easier :)
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
@alisonelizabeth @lockewritesdocs Thanks for addressing my feedback!
I did not re-test locally, but the proposed code changes look good to me.
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.
Looks good overall. I left some suggestions, but most are non-blocking nits.
One bigger discussion point: Most of the processors have an output field. Some can also update the input field in place.
We should develop a common pattern. Here are the ones we have so far:
Field to assign the converted value to. Defaults to ...
Field used to contain the converted value. Defaults to ...
Those are okay, but I think we may be able to get away with something simpler like:
Output field. If empty, the field is updated in place.
Regardless, we should pick one and use it consistently throughout. Let me know your thoughts. Once we make a decision, I can update the field help text in processors E-J.
...ation/components/pipeline_processors_editor/components/shared/map_processor_type_to_form.tsx
Outdated
Show resolved
Hide resolved
...components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx
Outdated
Show resolved
Hide resolved
...components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx
Outdated
Show resolved
Hide resolved
...components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx
Outdated
Show resolved
Hide resolved
...ation/components/pipeline_processors_editor/components/shared/map_processor_type_to_form.tsx
Outdated
Show resolved
Hide resolved
...ation/components/pipeline_processors_editor/components/shared/map_processor_type_to_form.tsx
Outdated
Show resolved
Hide resolved
...ents/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx
Outdated
Show resolved
Hide resolved
...ents/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx
Outdated
Show resolved
Hide resolved
...ation/components/pipeline_processors_editor/components/shared/map_processor_type_to_form.tsx
Outdated
Show resolved
Hide resolved
@@ -58,7 +59,7 @@ const fieldsConfig: Record<string, FieldConfig> = { | |||
helpText: ( | |||
<FormattedMessage | |||
id="xpack.ingestPipelines.pipelineEditor.dissectForm.appendSeparatorHelpText" | |||
defaultMessage="The character(s) that separate the appended fields. Default value is {value} (an empty string)." | |||
defaultMessage="Characters used to separate fields when appending two or more results together. Defaults to {value}." |
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.
This description didn't make sense to me. Is it used to separate or append results?
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 dug a little deeper on this field, and the append separator is only applicable when an append modifier is specified. I've updated the Pattern field to reference the key modifiers (including a link to that section of the doc page), and noted in the append separator description that it applies only if using a modifier in the pattern.
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
@elasticmachine merge upstream |
description: { | ||
defaultMessage: ( | ||
<FormattedMessage | ||
id="xpack.ingestPipelines.processors.description.dateIndexName" | ||
defaultMessage="Uses a date or timestamp to add documents to the correct time-based index. Index names must use a date math pattern, such as {value}." | ||
values={{ value: <EuiCode inline>{'my-index-yyyy-MM-dd'}</EuiCode> }} | ||
/> | ||
), | ||
}, |
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.
You will need to remove the { defaultMessage: ... }
wrapping the <FormattedMessage ... />
component - you you will probably also get a TS error about this :)
@jloleysens, I incorporated feedback from @jrodewig, so I think that these changes can now be merged. The only other change I recommend is removing the two horizontal bars for the Drop processor, since there are no additional fields to display. |
@lockewritesdocs Thanks for the updated! I will create a fix for that. |
@elasticmachine merge upstream |
merge conflict between base and head |
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
* master: [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751) Remove legacy deprecation adapter (elastic#76753) [Security Solution][Detections] Rule forms cleanup (elastic#76138) Adds back in custom images for reporting + tests (elastic#76810) [APM] Fix overlapping transaction names (elastic#76083) [Ingest Pipelines] Add descriptions for ingest processors A-D (elastic#75975)
…76113) (#76948) * [Ingest Pipelines] Add descriptions for ingest processors E-J (#76113) Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com> # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/shared/map_processor_type_to_form.tsx * Add code from #76948 * [Ingest Pipelines] Add descriptions for ingest processors A-D (#75975) * Fixing formatting issues identified by Prettier, part 2. * Fixing helpText labels. * Adding {value} object for dissect processor. * Incorporating reviewer feedback. * fix dropdown not rendering * Fixing typo. * add support for FormattedMessage in help text * fix TS * Updating some strings and trying to add code formatting. * fix formatted message * Editing some field descriptions. * Apply suggestions from code review Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com> * Trying to add EuiLink, plus edits. * fix help text for dissect processor * Incorporating reviewer feedback. * Trying to add another EUI element, plus edits. * fix date_index_name description text * Minor edit. * Fixing linter error. * Removing FunctionComponent, which was not read and caused build errors. Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com> Co-authored-by: Adam Locke <adam.locke@elastic.co> Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
This PR updates UI field strings and adds a description for the following processors, extending the work completed in #72849:
Relates to Ingest Node Processors Editors Forms
Relates to #76113 and #72849
How to test
Notes
There's a strong chance that the coding in
x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/shared/map_processor_type_to_form.tsx
is incorrect. @alisonelizabeth provided the proper syntax, but I might have broken something 😉Checklist
For maintainers