Skip to content

[ML] Converts index based data visualizer to React - #42685

Merged
peteharverson merged 5 commits into
elastic:masterfrom
peteharverson:ml-data-visualizer-react
Aug 8, 2019
Merged

peteharverson merged 5 commits into
elastic:masterfrom
peteharverson:ml-data-visualizer-react

Conversation

@peteharverson

@peteharverson peteharverson commented Aug 6, 2019 •

Copy link
Copy Markdown
Contributor

Summary

Migrates the index based data visualizer from AngularJS to React.

image

Functionally there is little change to the previous page. The most significant difference is the addition of a KQL based search bar, so that the page can be opened from a KQL based saved search as well as an index pattern.

image

If coming in from a lucene based saved search, the search string is displayed in a read-only search field:

image

The page now uses Elastic charts for charting document count and metric distributions.

See #18374

Note that unit tests will be added in a follow-up.

Checklist

For maintainers

@elasticmachine

Copy link
Copy Markdown
Contributor

Pinging @elastic/ml-ui

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good use of whilst 👍 😆

Comment thread x-pack/legacy/plugins/ml/public/data_visualizer/common/field_vis_config.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assuming these TODOs are going to be done in a follow up?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, using the inline styling of the series color in here and the document count chart will be done in a follow-up. It is awaiting the version of Elastic charts to be bumped to 8.0.0+

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@alvarezmelissa87 alvarezmelissa87 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Latest changes LGTM ⚡️

@walterra walterra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just added a suggestionsabout changing a function to get an array of JSX elements to a React component itself.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The content of most of the card content components is the same for each card, this could be moved to a common wrapping component to cut down on the duplication.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is enough variation in the content types that I would prefer to leave this for now, and consider it in a follow-up, for example when looking at switching the file data visualizer to use the same card component as this.

Comment thread x-pack/legacy/plugins/ml/public/data_visualizer/directive.tsx Outdated
Comment thread x-pack/legacy/plugins/ml/public/data_visualizer/directive.tsx Outdated
Comment thread x-pack/legacy/plugins/ml/public/data_visualizer/directive.tsx Outdated
Comment thread x-pack/legacy/plugins/ml/public/data_visualizer/page.tsx Outdated
Comment thread x-pack/legacy/plugins/ml/public/data_visualizer/data_loader/data_loader.ts Outdated
@peteharverson
peteharverson force-pushed the ml-data-visualizer-react branch from 31da825 to 64a2074 Compare August 8, 2019 10:04
@peteharverson
peteharverson force-pushed the ml-data-visualizer-react branch from 64a2074 to 992c67e Compare August 8, 2019 10:08

@jgowdyelastic jgowdyelastic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@peteharverson
peteharverson merged commit 900ca2a into elastic:master Aug 8, 2019
@peteharverson
peteharverson deleted the ml-data-visualizer-react branch August 8, 2019 11:48
peteharverson added a commit to peteharverson/kibana that referenced this pull request Aug 8, 2019
* [ML] Converts index based data visualizer to React

* [ML] Remove unused imports in React data visualizer

* [ML] Address review feedback

* [ML] Address comments from code review

* [ML] Remove redundant ts-ignore
peteharverson added a commit that referenced this pull request Aug 8, 2019
* [ML] Converts index based data visualizer to React

* [ML] Remove unused imports in React data visualizer

* [ML] Address review feedback

* [ML] Address comments from code review

* [ML] Remove redundant ts-ignore
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
* [ML] Converts index based data visualizer to React

* [ML] Remove unused imports in React data visualizer

* [ML] Address review feedback

* [ML] Address comments from code review

* [ML] Remove redundant ts-ignore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants