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] Converts index based data visualizer to React #42685

Merged

Conversation

peteharverson
Copy link
Contributor

@peteharverson peteharverson commented Aug 6, 2019

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
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

import { ML_BREADCRUMB } from '../breadcrumbs';

export function getDataVisualizerBreadcrumbs() {
// Whilst top level nav menu with tabs remains,
Copy link
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 👍 😆

yAccessors={['y']}
data={chartData.length > 0 ? chartData : [{ x: 0, y: 0 }]}
curve={CurveType.CURVE_STEP_AFTER}
// TODO - switch to use inline areaSeriesStyle to set series fill once charts version is 8.0.0+
Copy link
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
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
Contributor

💚 Build Succeeded

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM ⚡️

Copy link
Contributor

@walterra walterra left a comment

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.

}
}

export const BooleanContent: FC<Props> = ({ config }) => {
Copy link
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
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.

x-pack/legacy/plugins/ml/public/data_visualizer/page.tsx Outdated Show resolved Hide resolved
@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
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
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
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