-
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 Maps ui action to Index data visualizer/Discover's Field statistics #120846
Conversation
Pinging @elastic/ml-ui (:ml) |
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
...s/data_visualizer/public/application/common/components/field_data_row/action_menu/actions.ts
Outdated
Show resolved
Hide resolved
...n/index_data_visualizer/components/index_data_visualizer_view/index_data_visualizer_view.tsx
Outdated
Show resolved
Hide resolved
...s/data_visualizer/public/application/common/components/field_data_row/action_menu/actions.ts
Outdated
Show resolved
Hide resolved
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, thanks for adding a way to open geo fields in maps.
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
@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.
LGTM, nice and useful addon 👍 , tested and works as expected in Safari, Firefox, Chrome. Just identified a potential cleanup in the code
JSON.stringify({ | ||
searchQueryLanguage, | ||
searchString, | ||
}, | ||
actionFlyoutRef | ||
); | ||
if (!Array.isArray(actions) || actions.length < 1) return; | ||
|
||
const actionColumn: EuiTableActionsColumnType<FieldVisConfig> = { | ||
name: ( | ||
<FormattedMessage | ||
id="xpack.dataVisualizer.index.dataGrid.actionsColumnLabel" | ||
defaultMessage="Actions" | ||
/> | ||
), | ||
actions, | ||
width: '100px', | ||
}; | ||
|
||
return [actionColumn]; | ||
}, [currentIndexPattern, services, searchQueryLanguage, searchString]); | ||
|
||
}), |
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.
Just wondering if this is necessary, searchQueryLanguage
& searchString
are both strings, or am I missing something? so why building an object and then converting it to another 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.
Thanks for catching that 🙏 . The dependency previously includes a query object. I removed this now here 02db5e1
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @qn895 |
…tistics (elastic#120846) * Add map UI actions to dv * Fix maps logo, comment * Add permission check for ui actions * Remove unnecessary json stringify because it no longer includes query object Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
This PR adds the Maps uiAction to Index data visualizer/Discover's Field statistics:
Screen.Recording.2021-12-08.at.15.40.16.mov
Screen.Recording.2021-12-08.at.15.41.42.mov