Skip to content

[TSVB] Index pattern select field disappear in Annotation tab#102314

Merged
alexwizp merged 9 commits intoelastic:masterfrom
DianaDerevyankina:issues/102180
Jun 18, 2021
Merged

[TSVB] Index pattern select field disappear in Annotation tab#102314
alexwizp merged 9 commits intoelastic:masterfrom
DianaDerevyankina:issues/102180

Conversation

@DianaDerevyankina
Copy link
Contributor

@DianaDerevyankina DianaDerevyankina commented Jun 16, 2021

Closes #102180

Summary

Fixed missed index pattern field for the annotations panel.
The reason was no fetchedIndex provided for IndexPatternSelect.

annotations index pattern fix

annotations index pattern fix 2

For maintainers

@DianaDerevyankina DianaDerevyankina added Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 v7.14.0 labels Jun 16, 2021
@DianaDerevyankina DianaDerevyankina self-assigned this Jun 16, 2021
};
const changeFetchedIndex = async (index) => {
handleChange(index);
await this.setFetchedIndex(index[INDEX_PATTERN_NAME]);
Copy link
Contributor

Choose a reason for hiding this comment

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

how it should work in case of 2 or more annotations with different index patterns? Looks like you set in state the last fetchedIndex which will be used for each annotations.

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

I recommend you to put renderRow in a separate component. It should help you to solve that issue.

@DianaDerevyankina
Copy link
Contributor Author

@elasticmachine merge upstream

query_string: filter,
});

const onChange = (part: Partial<Annotation>) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

please use useCallback for these cases

import { getDefaultQueryLanguage } from './lib/get_default_query_language';
import type { Annotation, FetchedIndexPattern, IndexPatternValue } from '../../../common/types';
import type { AnnotationsEditorProps } from './annotations_editor';
import { newAnnotation } from './annotations_editor';
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid of cross references annotations_editor <---> annotations_ropw

import type { Annotation } from '../../../common/types';

interface YesNoProps<ParamName extends keyof TimeseriesVisParams> {
interface YesNoProps<ParamName extends keyof TimeseriesVisParams | keyof Annotation> {
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong type. not sure that YesNo component should know anything about Annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

and TimeseriesVisParams. Looks like bad typed Component. Can we use string instead?

defaultMessage="Click the button below to create an annotation data source."
/>
</p>
<EuiButton fill onClick={() => collectionActions.handleAdd(props, newAnnotation)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above, use useCallback

export interface AnnotationsEditorProps {
fields: VisFields;
model: Panel;
name: 'annotations';
Copy link
Contributor

Choose a reason for hiding this comment

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

what's a purpose of that field?

@alexwizp
Copy link
Contributor

image

@VladLasitsa
Copy link
Contributor

can we use the place more productively?
image

Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally in chrome

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM now

@alexwizp alexwizp marked this pull request as ready for review June 18, 2021 11:11
@alexwizp alexwizp requested a review from a team June 18, 2021 11:11
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Thanx for the "typescripting" 🙂
I tested both a new and an existing visualization with annotations and it works fine.
Just some nit picks, in general LGTM

</EuiFlexItem>
<EuiFlexItem>
<FieldSelect
type={'time_field'}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: No need to wrap it, type='time_field' and as we have done it for const INDEX_PATTERN_KEY = 'index_pattern'; we could do the same here

}
restrict={RESTRICT_FIELDS}
value={model.time_field}
onChange={handleChange('time_field')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, could use the same constant.


const NoContent = ({ handleAdd }: { handleAdd: () => void }) => (
<EuiText textAlign="center">
<p>
Copy link
Contributor

@stratoula stratoula Jun 18, 2021

Choose a reason for hiding this comment

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

Nit, do we need the extra p ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes... don't see any difference with and without p

Copy link
Contributor

@alexwizp alexwizp Jun 18, 2021

Choose a reason for hiding this comment

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

I was wrong it affects UI, let's keep it as before

image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 505 506 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeTimeseries 1.7MB 1.7MB +556.0B

History

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

cc @dziyanadzeraviankina

@alexwizp alexwizp merged commit 853de83 into elastic:master Jun 18, 2021
@alexwizp alexwizp self-assigned this Jun 18, 2021
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jun 18, 2021
…c#102314)

* [TSVB] Index pattern select field disappear in Annotation tab

* Refactor AnnotationsEditor and move renderRow logic into AnnotationRow

* Remove duplicated license, add useCallback to functions in AnnotationRow, remove cross refecrence and update types

* Refactor AnnotationEditor and AnnotationRow

* refactoring

* remove extra props

* fix nits

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jun 18, 2021
…c#102314)

* [TSVB] Index pattern select field disappear in Annotation tab

* Refactor AnnotationsEditor and move renderRow logic into AnnotationRow

* Remove duplicated license, add useCallback to functions in AnnotationRow, remove cross refecrence and update types

* Refactor AnnotationEditor and AnnotationRow

* refactoring

* remove extra props

* fix nits

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
# Conflicts:
#	src/plugins/vis_type_timeseries/public/application/components/yes_no.tsx
alexwizp added a commit that referenced this pull request Jun 18, 2021
…102314) (#102654)

* [TSVB] Index pattern select field disappear in Annotation tab (#102314)

* [TSVB] Index pattern select field disappear in Annotation tab

* Refactor AnnotationsEditor and move renderRow logic into AnnotationRow

* Remove duplicated license, add useCallback to functions in AnnotationRow, remove cross refecrence and update types

* Refactor AnnotationEditor and AnnotationRow

* refactoring

* remove extra props

* fix nits

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>
# Conflicts:
#	src/plugins/vis_type_timeseries/public/application/components/yes_no.tsx

* fix merge conflicts

Co-authored-by: Diana Derevyankina <54894989+DziyanaDzeraviankina@users.noreply.github.com>
alexwizp added a commit that referenced this pull request Jun 18, 2021
… (#102653)

* [TSVB] Index pattern select field disappear in Annotation tab

* Refactor AnnotationsEditor and move renderRow logic into AnnotationRow

* Remove duplicated license, add useCallback to functions in AnnotationRow, remove cross refecrence and update types

* Refactor AnnotationEditor and AnnotationRow

* refactoring

* remove extra props

* fix nits

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Alexey Antonov <alexwizp@gmail.com>

Co-authored-by: Diana Derevyankina <54894989+DziyanaDzeraviankina@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 21, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (447 commits)
  skip flaky suite (elastic#102366)
  [Security Solution][Endpoint][Host Isolation] Isolation status badge from alert details (elastic#102274)
  Add email connector info for Elastic Cloud (elastic#91363)
  [Workplace Search] remove or replace xs props for text on source connect view (elastic#102663)
  Do not double register dashboard url generator (elastic#102599)
  [TSVB] Replaces EuiCodeEditor 👉 Monaco editor  (elastic#100684)
  [Discover] Update kibana.json adding owner and description (elastic#102292)
  [Exploratory View] Mobile experience (elastic#99565)
  chore(NA): moving @kbn/ui-shared-deps into bazel (elastic#101669)
  [TSVB] Index pattern select field disappear in Annotation tab (elastic#102314)
  [Security Solution][Endpoint][Host Isolation] Fixes bug where host isolation/unisolation works from alert details (elastic#102581)
  TSVB visualizations with no timefield do not render after upgrading from 7.12.1 to 7.13.0 (elastic#102494)
  [Logs UI] Add `event.original` fallback to message reconstruction rules (elastic#102236)
  [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506)
  [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384)
  [ML] Anomaly detection job custom_settings improvements (elastic#102099)
  [Cases] Route: Get all alerts attach to a case (elastic#101878)
  Fixes wrong list exception type when creating endpoint event filters list (elastic#102522)
  remove search bar that's not working yet (elastic#102550)
  Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.13.3 v7.14.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TSVB] Index pattern select field disappear in Annotation tab

6 participants