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

[Ingest] Datastream list: add icons and dashboard links #65048

Merged
merged 14 commits into from
May 5, 2020

Conversation

skh
Copy link
Contributor

@skh skh commented May 4, 2020

Summary

Implements #64369

image

This adds the following features to the datastream list page:

  1. The package icon in the "Integration" column:
    image

The "Actions" column, and in it, links to dashboards in a nested context menu:
image

image
(see also "Known issues" below). See updates below.

When there is no dashboard, the link is greyed out:
image

When there is only one dashboard, the link is clickable, but there is no nested context menu (I faked the data to create this screenshot, our packages have either more than one dashboard or none):
image

Known issues

  • This PR currently uses the dashboard ids for display, i.e. the filenames of the dashboards as they are in the package. These are not very user friendly. I will try to fix this tomorrow, but want to open the PR now for review so people can comment now and I can make amends before feature freeze. See update below.

  • The PackageIcon component sometimes shows the default icon instead of the correct package icon. I'll try to find out why before FF.

How to test this

  • Enrol an agent to send data for a package that has dashboards
  • Navigate to "Data Streams"
  • Verify that you see an icon in the "Integrations" column
  • Use the Actions menu to navigate to dashboards

Update

The dashboard links use the dashboard titles from within the saved objects now:

image

@skh skh added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 Team:Fleet Team label for Observability Data Collection Fleet team labels May 4, 2020
@skh skh self-assigned this May 4, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@skh skh force-pushed the 64369-datastream-links-icons branch from 7e29bc6 to 95a0d71 Compare May 4, 2020 16:57
@skh skh marked this pull request as ready for review May 4, 2020 16:58
@skh skh requested a review from a team May 4, 2020 16:58
});
panels.push({
id: 1,
title: 'View dashboards',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gave a type error from somewhere within EUI when I used a <FormattedMessage /> instead, so this is not translated. I'm sure there's a way to fix that, comments welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use i18n.translate instead of FormattedMessage? the prop might expect a string instead of react node

<EuiButtonIcon
iconType="boxesHorizontal"
onClick={handleToggleMenu}
aria-label={i18n.translate('xpack.ingestManager.genericActionsMenuTextB', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
aria-label={i18n.translate('xpack.ingestManager.genericActionsMenuTextB', {
aria-label={i18n.translate('xpack.ingestManager.genericActionsMenuText', {

(small typo?)

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Small comments but otherwise code LGTM! Testing locally now.

@@ -23,7 +23,7 @@ export const TableRowActions = React.memo<{ items: EuiContextMenuPanelProps['ite
<EuiButtonIcon
iconType="boxesHorizontal"
onClick={handleToggleMenu}
aria-label={i18n.translate('xpack.ingestManager.agentConfigList.actionsMenuText', {
aria-label={i18n.translate('xpack.ingestManager.agentConfigList.ActionsMenuText', {
Copy link
Contributor

Choose a reason for hiding this comment

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

revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Thanks.

});
panels.push({
id: 1,
title: 'View dashboards',
Copy link
Contributor

Choose a reason for hiding this comment

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

needs i18n

@@ -8196,7 +8196,6 @@
"xpack.ingestManager.agentConfigForm.systemMonitoringText": "システムメトリックを収集",
"xpack.ingestManager.agentConfigForm.systemMonitoringTooltipText": "このオプションを有効にすると、システムメトリックと情報を収集するデータソースで構成をブートストラップできます。",
"xpack.ingestManager.agentConfigList.actionsColumnTitle": "アクション",
"xpack.ingestManager.agentConfigList.actionsMenuText": "開く",
Copy link
Contributor

Choose a reason for hiding this comment

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

these two translation files may need to be reverted if you revert the change to the i18n key

Comment on lines +127 to +128
package: pkg,
package_version: packageMetadata[pkg] ? packageMetadata[pkg].version : '',
Copy link
Contributor

Choose a reason for hiding this comment

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

After testing locally, I realized we still display the package id instead of the nice name (system vs System). Now that we have the package saved objects here, what do you think about restructuring DataStream type to include package name?:

package: {
  id: string;
  name: string;
  version: string;
}

Not a blocker for FF tho :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should do that for beta1. We'll need to add the title from the registry package information to our epm-packages saved object type, and I'm reluctant to do that right now, so this is #65230 now.

@ruflin ruflin added the Ingest Management:alpha1 Group issues for ingest management alpha1 label May 5, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@skh skh merged commit 4142f57 into elastic:master May 5, 2020
@skh skh deleted the 64369-datastream-links-icons branch May 5, 2020 11:55
skh added a commit to skh/kibana that referenced this pull request May 5, 2020
* Read package saved objects in data stream handler.

* Render package icon.

* Make TableRowAction more generic

* Add Actions column to data stream list

* Disable dashboard link if no dashboards present.

* Data stream list: link to first dashbord found

* Update i18n strings

* Add nested context menu to link to dashboards

* introduces a separate TableRowActionsNested component
* moves TableRowActions back into agent config components

* Fix i18n label.

* Re-add translated strings removed by mistake

* Fix i18n issues

* Add helper to read a saved object installed by EPM

* Display titles from within dashboard saved objects
skh added a commit that referenced this pull request May 5, 2020
)

* Read package saved objects in data stream handler.

* Render package icon.

* Make TableRowAction more generic

* Add Actions column to data stream list

* Disable dashboard link if no dashboards present.

* Data stream list: link to first dashbord found

* Update i18n strings

* Add nested context menu to link to dashboards

* introduces a separate TableRowActionsNested component
* moves TableRowActions back into agent config components

* Fix i18n label.

* Re-add translated strings removed by mistake

* Fix i18n issues

* Add helper to read a saved object installed by EPM

* Display titles from within dashboard saved objects
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 5, 2020
* master: (133 commits)
  Cleanup Typescript index pattern field editor / Expression functions for bucket agg (elastic#65254)
  Removes legacy infra plugin and moves saved objects registration to NP (elastic#64848)
  Added support for docLinks plugin in Connectors forms and missing save capabilities for modal dialog (elastic#64986)
  [SIEM] Removes prebuilt rules number dependency (elastic#65128)
  [Maps] add categorical palettes with 20 and 30 categories (elastic#64701)
  [CI] Slack alerts - Elasticsearch snapshot failures (elastic#64724)
  [Uptime] Console errors in case index missing (elastic#65115)
  [SIEM][CASE] Dynamic fields mapping based on connector (elastic#64412)
  [test/functional] Tsfy page objects (elastic#64887)
  [Maps] [Telemetry] Track geo_point and geo_shape index patterns separately (elastic#65195)
  [Maps] Add global fit to data (elastic#64702)
  Visualize: Reload on ui state change and fix ui state for tsvb (elastic#63699)
  [SIEM] [Cases] External service selection per case (elastic#64775)
  [Uptime] Set ML anomaly look-back to 2w (from 24h) / Add spinner (elastic#65055)
  [Metrics UI] Remove APM Hard Dependency (elastic#64952)
  [Ingest] Datastream list: add icons and dashboard links (elastic#65048)
  disable plugins. they could access ES via SO repository (elastic#65242)
  Feature fleet enrollment instructions (elastic#65176)
  [SIEM] Adds 'Configure connector' Cypress test (elastic#64807)
  [TSVB] Fix std deviation band mode (elastic#64413)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ingest Management:alpha1 Group issues for ingest management alpha1 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants