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

Shim input_control_vis for KP #52243

Merged
merged 44 commits into from
Dec 18, 2019

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Dec 5, 2019

Summary

  • Shim input_control_vis plugin for kibana platform
  • Convert plugin to typescript

Checklist

For maintainers

@nickofthyme nickofthyme added Feature:New Platform v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Dec 5, 2019
- Update Filter types with RangeField and PhaseFilter
- Fix broken types in input_control_vis tests
@elastic elastic deleted a comment from elasticmachine Dec 5, 2019
@elastic elastic deleted a comment from elasticmachine Dec 10, 2019
@elastic elastic deleted a comment from elasticmachine Dec 10, 2019
@elastic elastic deleted a comment from elasticmachine Dec 10, 2019
@nickofthyme
Copy link
Contributor Author

@elasticmachine merge upstream

@nickofthyme
Copy link
Contributor Author

@elasticmachine merge upstream

async getIndexPatternSelect() {
const [, { data }] = await this.props.deps.core.getStartServices();
this.setState({
IndexPatternSelect: data.ui.IndexPatternSelect,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes :)

return this.indexPattern;
}

getField() {
getField(): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

id: indexPatternId,
fields: {
getByName: name => {
const fields = { field1: fieldMock };
getByName: (name: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Just a small note, the rest LGTM. Tested and visualization still works

@@ -144,7 +158,7 @@ class FieldSelectUi extends Component {
label={
<FormattedMessage
id="inputControl.editor.fieldSelect.fieldLabel"
defaultMessage="Field"
defaultMessage="IFieldType"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this happened accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Joe!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

createFilter(phrases: any): esFilters.PhraseFilter {
let newFilter: esFilters.PhraseFilter;
// TODO: Fix type to be required
const value = this.indexPattern.fields.getByName(this.fieldName) as IFieldType;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thing type is dynamically inferred here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya the method may return undefined and the js was not checking that. I added a check to throw if doesn't exist.

const value = this.indexPattern.fields.getByName(this.fieldName);

if (!value) {
  throw new Error(`unable to find fieldName: ${this.fieldName} on indexPattern`);
}

if (phrases.length === 1) {
  newFilter = esFilters.buildPhraseFilter(value, phrases[0], this.indexPattern);
} else {
  newFilter = esFilters.buildPhrasesFilter(value, phrases, this.indexPattern);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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


controls: Array<RangeControl | ListControl>;
queryBarUpdateHandler: () => void;
filterManager: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use type from data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I wasn't sure about this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -18,10 +18,10 @@
*/

import dateMath from '@elastic/datemath';
import { TimeRange } from '../../../common';
import { IIndexPattern } from 'src/plugins/data/public';
Copy link
Contributor

Choose a reason for hiding this comment

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

The import is from within the plugin
I think you could do import { IIndexPattern } from '../..';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks my auto import pulls from the aliases by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

in terms of @elastic/kibana-app-arch service usage LGTM
(didn't checkout after updates)

@lizozom lizozom mentioned this pull request Dec 18, 2019
7 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@nickofthyme nickofthyme merged commit 72c5c5c into elastic:master Dec 18, 2019
@nickofthyme nickofthyme deleted the kpm/control-input-shim branch December 18, 2019 22:25
nickofthyme added a commit to nickofthyme/kibana that referenced this pull request Dec 19, 2019
* Shim input_control_vis
* Convert input_control_vis src files to typescript
* Add Required, Optional, Required and Class types to kbn-utility-types
* Collect all ui/* imports into legacy imports file
* Pass down plugin deps from top level
* Add timeout and terminate_after options to SearchSourceFields
nickofthyme added a commit that referenced this pull request Dec 19, 2019
* Shim input_control_vis
* Convert input_control_vis src files to typescript
* Add Required, Optional, Required and Class types to kbn-utility-types
* Collect all ui/* imports into legacy imports file
* Pass down plugin deps from top level
* Add timeout and terminate_after options to SearchSourceFields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants