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.

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

[ML] Transforms: Fixes available fields for sort options for latest configuration #88617

Merged
merged 9 commits into from
Jan 21, 2021

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Jan 18, 2021

Summary

  • Fixes the transform preview header to display the heading text in any case and the copy-to-clipboard button for latest configurations (the copy-to-clipboard option for pivot is displayed within the form).
  • Fix to avoid listing all fields for the sort option for latest configuration and only show date fields
  • Fixes ambiguous form field labels

Checklist

For maintainers

@walterra walterra added bug Fixes for quality problems that affect the customer experience :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Transforms ML transforms v7.11.0 v7.12.0 labels Jan 18, 2021
@walterra walterra self-assigned this Jan 18, 2021
@walterra walterra requested a review from a team as a code owner January 18, 2021 17:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@@ -56,7 +56,8 @@ function getOptions(
}));

const sortFieldOptions: Array<EuiComboBoxOptionOption<string>> = indexPattern.fields
.filter((v) => !ignoreFieldNames.has(v.name) && v.sortable)
// The backend API for `latest` allows all field types for sort but the UI will be limited to `date`.
.filter((v) => !ignoreFieldNames.has(v.name) && v.sortable && v.type === 'date')
Copy link
Contributor

Choose a reason for hiding this comment

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

Any ideas what we should do if there are no date fields in the source index?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative solution could be to still make all fields available but group them in the dropdown by field type, with date fields first, then numbers, then keyword.

Something like

date fields

  @timestamp
  created_at

----
number fields

  responsetime

----
other

  airline

Screenshot of the grouped dropdown from EUI docs:

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, let's stick to the current implementation and only show date type fields in the dropdown. Advanced users can switch to the API or dev tools if they really want to use a number field say. And add some help text below the Sort field input, along the lines of Select the date field to be used to identify the latest document.

Copy link
Contributor

Choose a reason for hiding this comment

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

That text sounds fine to me. I wonder if we can alternatively change the "Add a sort field...." text in the screenshot to "Add a date field ...." and maybe even "...that identifies the latest documents" if space allows.

Co-authored-by: Lisa Cawley <[email protected]>
Comment on lines +391 to +392
copyToClipboard={copyToClipboardPivot}
copyToClipboardDescription={copyToClipboardPivotDescription}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider passing the "Copy to clipboard" component as a child instead of adding extra 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.

It's the same prop structure how we pass it on to DataGrid so didn't want to change that for this PR.

@walterra
Copy link
Contributor Author

Changed latestConfigMapper.toAPIConfig() to return partial configs similar to pivots so a user can copy a config which is not complete (e.g. if no date field for sort is available) to the clipboard to continue in Dev Console. That required to update validateLatestConfig too. I added some jest tests to cover the changes.

@walterra
Copy link
Contributor Author

@lcawl thanks for the feedback, updated the texts.

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest edits and LGTM

@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
ml 7.1MB 7.1MB +57.0B
transform 1.0MB 1.0MB +2.3KB
total +2.3KB

History

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

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

@walterra walterra merged commit 922abfa into elastic:master Jan 21, 2021
@walterra walterra deleted the ml-transforms-latest-fix-sort branch January 21, 2021 07:12
walterra added a commit to walterra/kibana that referenced this pull request Jan 21, 2021
…onfiguration (elastic#88617)

- Fixes the transform preview header to display the heading text in any case and the copy-to-clipboard button for latest configurations (the copy-to-clipboard option for pivot is displayed within the form).
- Fix to avoid listing all fields for the sort option for latest configuration and only show date fields
- Fixes ambiguous form field labels
walterra added a commit to walterra/kibana that referenced this pull request Jan 21, 2021
…onfiguration (elastic#88617)

- Fixes the transform preview header to display the heading text in any case and the copy-to-clipboard button for latest configurations (the copy-to-clipboard option for pivot is displayed within the form).
- Fix to avoid listing all fields for the sort option for latest configuration and only show date fields
- Fixes ambiguous form field labels
walterra added a commit that referenced this pull request Jan 21, 2021
…onfiguration (#88617) (#88934)

- Fixes the transform preview header to display the heading text in any case and the copy-to-clipboard button for latest configurations (the copy-to-clipboard option for pivot is displayed within the form).
- Fix to avoid listing all fields for the sort option for latest configuration and only show date fields
- Fixes ambiguous form field labels
walterra added a commit that referenced this pull request Jan 21, 2021
…onfiguration (#88617) (#88933)

- Fixes the transform preview header to display the heading text in any case and the copy-to-clipboard button for latest configurations (the copy-to-clipboard option for pivot is displayed within the form).
- Fix to avoid listing all fields for the sort option for latest configuration and only show date fields
- Fixes ambiguous form field labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Transforms ML transforms :ml release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants