Clean up TSVB type client code to conform to the schema by DianaDerevyankina · Pull Request #68519 · elastic/kibana

DianaDerevyankina · 2020-06-08T14:24:51Z

Part of #57342

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

Part of elastic#57342

sulemanof · 2020-06-11T14:44:23Z

src/plugins/vis_type_timeseries/common/types.ts

+  nested?: { path: string };
+}
+
+export interface FieldDescriptor {


I assume IFieldType from data plugin is fully suitable for TSVB, isn't it?

sulemanof · 2020-06-11T14:58:36Z

src/plugins/vis_type_timeseries/public/application/components/add_delete_buttons.tsx

+  onDelete: (event: MouseEvent) => void;
+}
+
+export function AddDeleteButtons(props: AddDeleteButtonsProps) {


Does it make sense to type in addition the test file for this component: add_delete_buttons.test.js ?

sulemanof · 2020-06-11T15:06:54Z

src/plugins/vis_type_timeseries/public/application/components/aggs/agg.tsx

+  panel: PanelSchema;
+  series: SeriesItemsSchema;
+  siblings: MetricsItemsSchema[];
+  uiRestrictions: { '*': boolean };


There is a list of possible ui restrictions keys exist in src/plugins/vis_type_timeseries/common/ui_restrictions.ts.
Could you please add them as possible keys?

# Conflicts: # src/plugins/vis_type_timeseries/common/post_vis_schema.ts

alexwizp · 2020-06-16T07:59:30Z

src/plugins/vis_type_timeseries/public/application/components/add_delete_buttons.test.tsx

 */

 import React from 'react';
+// @ts-ignore


I think import { expect } from 'chai'; was added by mistake. Please remove it

alexwizp · 2020-06-16T08:19:23Z

src/plugins/vis_type_timeseries/public/application/components/aggs/agg_select.tsx


-  let enablePipelines = siblings.some((s) => !!metricAggs.find((m) => m.value === s.type));
+  let enablePipelines = siblings.some(
+    (s: { id: string; type: string }) => !!metricAggs.find((m) => m.value === s.type)


Please don't add extra types like e.g. { id: string; type: string }. You already set type for siblings variable.

elasticmachine · 2020-06-17T08:09:58Z

Pinging @elastic/kibana-app (Team:KibanaApp)

alexwizp · 2020-06-18T09:22:30Z

@elasticmachine merge upstream

alexwizp

Good first step of migrating js -> ts

alexwizp · 2020-06-23T07:20:20Z

@elasticmachine merge upstream

sulemanof

Left few comments, but overall LGTM

sulemanof · 2020-06-23T07:26:59Z

src/plugins/vis_type_timeseries/common/ui_restrictions.ts

+export interface TimeseriesUIRestrictions {
+  [RESTRICTIONS_KEYS.WHITE_LISTED_GROUP_BY_FIELDS]: Record<string, UIRestrictions>;
+  [RESTRICTIONS_KEYS.WHITE_LISTED_METRICS]: Record<string, UIRestrictions>;
+  [RESTRICTIONS_KEYS.WHITE_LISTED_TIMERANGE_MODES]: Record<string, UIRestrictions>;
+}


Will this work for your case?

Suggested change

export interface TimeseriesUIRestrictions {

[RESTRICTIONS_KEYS.WHITE_LISTED_GROUP_BY_FIELDS]: Record<string, UIRestrictions>;

[RESTRICTIONS_KEYS.WHITE_LISTED_METRICS]: Record<string, UIRestrictions>;

[RESTRICTIONS_KEYS.WHITE_LISTED_TIMERANGE_MODES]: Record<string, UIRestrictions>;

}

export type TimeseriesUIRestrictions = {

[key in RESTRICTIONS_KEYS]: Record<string, UIRestrictions>;

};

sulemanof · 2020-06-23T07:34:56Z

...ype_timeseries/public/application/components/aggs/percentile_rank/percentile_rank_values.tsx


-export const PercentileRankValues = (props) => {
+interface PercentileRankValuesProps {
+  model: any[];


Doesnt it receive MetricsItemsSchema['values']; ?

sulemanof · 2020-06-23T07:37:19Z

...s/vis_type_timeseries/public/application/components/aggs/percentile_rank/percentile_rank.tsx

+  const handlePercentileRankValuesChange = (values: Array<string | null> | null | undefined) => {
    handleChange(
      assign({}, model, {
        values,


Suggested change

const handlePercentileRankValuesChange = (values: Array<string | null> | null | undefined) => {

handleChange(

assign({}, model, {

values,

const handlePercentileRankValuesChange = (values: MetricsItemsSchema['values']) => {

handleChange(

assign({}, model, {

values,

You could also get rid of lodash here (we are trying to do it if possible):

handleChange({ ...model, values })

stratoula · 2020-06-23T10:08:24Z

...s/vis_type_timeseries/public/application/components/aggs/percentile_rank/multi_value_row.tsx

  const htmlId = htmlIdGenerator();

-  const onFieldNumberChange = (event) =>
+  const onFieldNumberChange = (event: any) =>


Can we avoid this any ?

stratoula · 2020-06-23T10:14:47Z

...s/vis_type_timeseries/public/application/components/aggs/percentile_rank/multi_value_row.tsx

          <EuiFieldNumber
            value={model.value === '' ? '' : Number(model.value)}
-            placeholder={0}
+            placeholder={'0'}


Nit: you could also omit curly brackets with "0"

stratoula

LGTM, nice first touch :) I have also tested it on Chrome. Works as expected

…tions

alexwizp · 2020-06-24T14:03:37Z

@elasticmachine merge upstream

kibanamachine · 2020-06-24T15:56:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3cd16b2

Build metrics

✅ unchanged

History

💔 Build #55855 failed bf4dba3
💛 Build #55446 was flaky 1c11748
💚 Build #55298 succeeded 7085e58
💚 Build #54947 succeeded c488b2c
💚 Build #54313 succeeded 113c111

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

* Clean up TSVB type client code to conform to the schema Part of elastic#57342 * Replace FieldDescriptor with IFieldType, add UIRestrictions interface * Replace expect from chai with @kbn/expect, remove unnecessary type * Add TimeseriesUIRestrictions type and refactor add_delete_buttons.test * Replace some types with MetricsItemsSchema['values'] to avoid duplications Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (45 commits) [QA] Unskip functional tests (elastic#69760) [SIEM][Detection Engine] - Update DE to work with new exceptions schema (elastic#69715) Fixes elastic#69639: Ignore url.url fields above 2048 characters (elastic#69863) PR: Provide limit warnings to user when API limits are reached. (elastic#69590) [Maps] Remove broken button (elastic#69853) Makes usage collection methods available on start (elastic#69836) [SIEM][CASE] Improve Jira's labelling (elastic#69892) [Logs UI] Access ML via the programmatic plugin API (elastic#68905) [ML] DF Analytics: Creation wizard part 3 (elastic#69456) Update Resolver generator script documentation (elastic#69912) [ML] Changes View results button text on new job page (elastic#69809) Add master branch to backport config (elastic#69893) [Ingest Manager] Kibana, not EPR, controls removable packages (elastic#69761) unskips 'Events columns' test (elastic#69684) [ML] Changes the ML overview empty analytics panel text (elastic#69801) [DOCS] Emphasizes where File Data Visualizer is located. (elastic#69812) add the `exactRoute` property to app registration (elastic#69772) Bump backport to 5.4.6 (elastic#69880) [Logs UI] ML log integration splash screen (elastic#69288) Clean up TSVB type client code to conform to the schema (elastic#68519) ...

) * Clean up TSVB type client code to conform to the schema Part of #57342 * Replace FieldDescriptor with IFieldType, add UIRestrictions interface * Replace expect from chai with @kbn/expect, remove unnecessary type * Add TimeseriesUIRestrictions type and refactor add_delete_buttons.test * Replace some types with MetricsItemsSchema['values'] to avoid duplications Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Clean up TSVB type client code to conform to the schema

6995983

Part of elastic#57342

DianaDerevyankina added Feature:TSVB TSVB (Time Series Visual Builder) v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 8, 2020

DianaDerevyankina self-assigned this Jun 8, 2020

DianaDerevyankina requested review from VladLasitsa, alexwizp and sulemanof June 11, 2020 08:15

sulemanof reviewed Jun 11, 2020

View reviewed changes

DianaDerevyankina added 2 commits June 12, 2020 13:21

Merge branch 'DianaDerevyankina' into Diana/57342-1

bbe92a9

# Conflicts: # src/plugins/vis_type_timeseries/common/post_vis_schema.ts

Replace FieldDescriptor with IFieldType, add UIRestrictions interface

d67172d

alexwizp reviewed Jun 16, 2020

View reviewed changes

alexwizp requested a review from stratoula June 16, 2020 08:16

alexwizp reviewed Jun 16, 2020

View reviewed changes

DianaDerevyankina added 2 commits June 16, 2020 11:37

Merge branch 'DianaDerevyankina' into Diana/57342-1

dc85566

Replace expect from chai with @kbn/expect, remove unnecessary type

113c111

DianaDerevyankina added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Jun 17, 2020

DianaDerevyankina requested a review from flash1293 June 17, 2020 08:09

DianaDerevyankina marked this pull request as ready for review June 17, 2020 08:09

DianaDerevyankina requested a review from a team June 17, 2020 08:09

Merge branch 'master' into Diana/57342-1

c488b2c

alexwizp approved these changes Jun 18, 2020

View reviewed changes

DianaDerevyankina added 2 commits June 19, 2020 11:35

Merge branch 'DianaDerevyankina' into Diana/57342-1

1b62b5b

Add TimeseriesUIRestrictions type and refactor add_delete_buttons.test

7085e58

Merge branch 'master' into Diana/57342-1

1c11748

sulemanof approved these changes Jun 23, 2020

View reviewed changes

stratoula reviewed Jun 23, 2020

View reviewed changes

stratoula approved these changes Jun 23, 2020

View reviewed changes

Replace some types with MetricsItemsSchema['values'] to avoid duplica…

bf4dba3

…tions

Merge branch 'master' into Diana/57342-1

3cd16b2

DianaDerevyankina merged commit 2b72de5 into elastic:master Jun 25, 2020

DianaDerevyankina mentioned this pull request Jun 25, 2020

[7.x] Clean up TSVB type client code to conform to the schema (#68519) #69885

Merged

alexwizp mentioned this pull request Aug 27, 2021

[TSVB] Remove deprecated IFieldType #110404

Merged

1 task

Conversation

DianaDerevyankina commented Jun 8, 2020

Summary

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 17, 2020

Uh oh!

alexwizp commented Jun 18, 2020

Uh oh!

alexwizp left a comment

Choose a reason for hiding this comment

Uh oh!

alexwizp commented Jun 23, 2020

Uh oh!

sulemanof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

alexwizp commented Jun 24, 2020

Uh oh!

kibanamachine commented Jun 24, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants