-
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
[ML] Add index pattern management to index Data Visualizer #101316
[ML] Add index pattern management to index Data Visualizer #101316
Conversation
5e4c520
to
fbbf1e9
Compare
fbbf1e9
to
e5385bd
Compare
@elasticmachine merge upstream |
}); | ||
} | ||
|
||
// Allow to edit index pattern field |
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 see in Discover you get the option to delete runtime fields that have been added to the index pattern? Did you look into providing that functionality? Not a requirement for this PR, but just wondered how much work that would involve?
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.
Note as far I'm aware, the delete action should only be added for runtime fields defined in the index pattern.
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.
Added the delete index pattern field option here if it's a runtime field and if user has edit index pattern permission. Note that the current EUI behavior automatically makes it a dropdown menu if there's > 2 actions. This seems to be a strict EUI guideline so I think we should revisit where would be the best place for the delete
button
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 think this looks ok, and follows the EUI guidelines with the ellipses icon used when there are more than 2 actions.
I have updated the PR to fix this issue here. Also added the index pattern deletion flyout although I don't think the current UI is very ideal. |
Pinging @elastic/ml-ui (:ml) |
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.
Tested latest changes and LGTM
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.
Added a couple of nit pick comments but generally LGTM
actionFlyoutRef.current = indexPatternFieldEditor?.openEditor({ | ||
ctx: { indexPattern }, | ||
fieldName: item.fieldName, | ||
onSave: () => { |
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.
nit, onSave
and onDelete
are the same so could be moved to a common function
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.
Updated here dcd6cfa
services: { lens: lensPlugin, docLinks, notifications, uiSettings }, | ||
} = useDataVisualizerKibana(); | ||
const { services } = useDataVisualizerKibana(); | ||
const { docLinks, notifications, uiSettings } = services; |
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 could all be done in one statement. but i'm not sure which is better
const {
services,
services: {
docLinks,
notifications: { toasts },
uiSettings,
},
} = useDataVisualizerKibana();`
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 think I unpacked the services here because it's being used later in getActions
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 to see functional tests coming as part of this PR 👏
Just left a few comments / suggestions.
Checking test stability in a flaky test runner job ... no failure in 50 runs ✔️ |
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.
lgtm, great to see this being implemented!
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.
Functional tests LGTM 🎉
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.
AppServices changes LGTM.
@elasticmachine merge upstream |
Checking test stability in a flaky test runner job ... no failure in 50 runs ✅ |
…01316) * [ML] Add index pattern editor flyout * [ML] Add indexPatternField editor plugin as opt dependency * [ML] Remove lens from ML's dependency * [ML] Fix custom display name cause field to be missing * [ML] Add delete option * [ML] Fix aggregatableFields logic * [ML] Add functional tests * [ML] Fix labels & consolidate addRuntimeFields * [ML] Add tooltip to show or hide distributions * Consolidate refreshPage * [ML] Fix tests Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…103156) * [ML] Add index pattern editor flyout * [ML] Add indexPatternField editor plugin as opt dependency * [ML] Remove lens from ML's dependency * [ML] Fix custom display name cause field to be missing * [ML] Add delete option * [ML] Fix aggregatableFields logic * [ML] Add functional tests * [ML] Fix labels & consolidate addRuntimeFields * [ML] Add tooltip to show or hide distributions * Consolidate refreshPage * [ML] Fix tests Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Quynh Nguyen <[email protected]>
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_lifecycle_management/policies·js.apis management index lifecycle management policies list should have a default policy to manage the Watcher history indicesStandard Out
Stack Trace
Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_lifecycle_management/policies·js.apis management index lifecycle management policies list should have a default policy to manage the Watcher history indicesStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @qn895 |
Summary
Part of #101435 and follow up of #100922. This PR adds the index pattern field editor flyout to the Data Visualizer. New functionalities include:
Add new index pattern field
![2021-06-06 at 22 40](https://user-images.githubusercontent.com/43350163/120956047-60140780-c718-11eb-9c25-8e60eaa8d63b.gif)
Edit index pattern field
![2021-06-06 at 22 34](https://user-images.githubusercontent.com/43350163/120955625-69e93b00-c717-11eb-904c-1f2c77a4c458.gif)
Manage index pattern fields
![2021-06-06 at 22 36](https://user-images.githubusercontent.com/43350163/120955740-afa60380-c717-11eb-8d42-3f82fe6cd56b.gif)
Delete index pattern field
Added tooltip for the Show/Hide distribution button
Review Hints:
data-test-subj
for index pattern editor form to help with functional testsDelete any items that are not applicable to this PR.