[Lens] Manual Annotations #126456

mbondyra · 2022-02-28T13:14:12Z

Summary

Introduces new type of the layer for xy chart: annotations.
Only targets manual, 'point in time' annotations.

The icon set is covering most cases for tsvb (to be able to migrate in the future) and all we use for reference lines
Ranges will be added soon (in separate pr).
Drag and drop will be added soon (in separate pr).
Tooltip collect the info for all the annotations from the interval.

Default look:

Grouping and custom annotations:

Event annotations service

At the moment the service only contains the expressions.

mbondyra · 2022-03-10T14:11:53Z

@elasticmachine merge upstream

alexwizp · 2022-03-23T09:49:02Z

src/plugins/event_annotation/public/event_annotation_service/helpers.ts

+ * Side Public License, v 1.
+ */
+import { euiLightVars } from '@kbn/ui-theme';
+export const defaultAnnotationColor = euiLightVars.euiColorAccent;


nit: I probably recommended not to fix the value at the time of import. Maybe geDefaultAnnotationColor = () => euiLightVars.euiColorAccent;

Why does it matter? 🤔

@mbondyra not related to alex suggestion, but to the color: the euiColorAccent differs if you are in dark mode or light mode. in the light theme the color is #F04E98 but is #F68FBE in dark mode.
@MichaelMarcialis ideas?

@markov00 does it matter? Why is it concerning?

On switching to dark mode the annotations will still have the light theme color because it's putting the hex value into the saved object. I think that is OK for this PR, it's the same for all of the place where we allow the users to pick a color (series, reference lines and so on). When we discussed this the last time we said we would tackle this problem separately in a general way and apply the same solution to all the places which allow this kind of thing right now.

@mbondyra not related to alex suggestion, but to the color: the euiColorAccent differs if you are in dark mode or light mode. in the light theme the color is #F04E98 but is #F68FBE in dark mode.
@MichaelMarcialis ideas?

@markov00: That's a good point; something we hadn't considered in our discussion yesterday. Ideally, we would have a single color value that works well for both light and dark modes. Otherwise, the default color is not a purely static value, but a partially dynamic one that changes depending on the theme mode selected when the visualization was created. This could be unexpected or undesirable to users (especially after changing an existing space's theme mode, when sharing/copying visualization across spaces, and in the future when users can set their own theme modes). This is part of the reason why we chose a gray color for the default reference line color, because it works in both theme modes.

In this case, the difference in the color value between light and dark mode presents a similar problem to the one we discussed yesterday if we were to choose black (which would require switching to white in dark mode). That said, because the contrast gap between accent colors in light and dark mode is far lower than the contrast gap between black and white, perhaps we can live with it for now until a more general solution for handling color selection between light/dark themes is addressed, as @flash1293 stated above.

For the sake of the immediate present, I'd say let's either stick with the EUI accent color (regardless of the fact that it will change on visualization creation depending on theme mode), or switch to same gray we use for reference lines as the default.

src/plugins/event_annotation/public/plugin.ts

alexwizp · 2022-03-23T10:06:55Z

src/plugins/event_annotation/public/plugin.ts

+  public setup(core: CoreSetup, dependencies: SetupDependencies): EventAnnotationPluginSetup {
+    dependencies.expressions.registerFunction(manualEventAnnotation);
+    dependencies.expressions.registerFunction(eventAnnotationGroup);
+    return this.eventAnnotationService;


nit: I see only one method in that service, not sure that it's a legal to execute this one during setup phase. I prefer to return {} or cleanup API

But at some point I'll add extra functionality here. I will not correct it in this PR but revisit later to make sure if it doesn't break any rules.

x-pack/plugins/lens/public/xy_visualization/annotations/config_panel/index.tsx

x-pack/plugins/lens/public/xy_visualization/annotations/helpers.tsx

flash1293

It seems like there are still a few references to explicit icon placement in the helpers, do we still need those?

kibana/x-pack/plugins/lens/public/xy_visualization/annotations_helpers.tsx

Line 57 in fc618c1

    
           Pick<YConfig, 'axisMode' | 'icon' | 'iconPosition' | 'textVisibility'> | undefined

In general I really like this change, one thing we can also split out - what if on creating a new annotation it would copy over the styling from the last one in the list? That would make it easy to create lots of annotations which are similarly styled (set the styling on the first one and it will be pre-set for the rest)

mbondyra · 2022-03-23T13:03:35Z

@flash1293 about the references to icon position, it happens because the helpers are shared between reference lines and annotations so for reference lines it's still needed.

Copying styles from the last one in the list/ last one edited - what do you think about it @gvnmagni @MichaelMarcialis? Nevertheless, I'd address it in a separate PR.

x-pack/plugins/maps/public/lens/choropleth_chart/region_key_editor.tsx

gvnmagni · 2022-03-23T14:56:34Z

@flash1293 about the references to icon position, it happens because the helpers are shared between reference lines and annotations so for reference lines it's still needed.

Copying styles from the last one in the list/ last one edited - what do you think about it @gvnmagni @MichaelMarcialis? Nevertheless, I'd address it in a separate PR.

The concept itself is really interesting, the only concern I have is that we have to find a proper way to explain what's happening to the user and I'm not sure there is a simple way to do that, I'll hear what Michael has to say

flash1293

LGTM, talked through a few small things offline and this is a great first version

…otation_service

nreese

maps changes LGTM
code review, tested in chrome

stratoula

Changes LGTM, sorry for the delay Marta. I tested everything and it works fine. Great PR, great enhancement ❤️

x-pack/plugins/lens/public/app_plugin/app.scss

MichaelMarcialis · 2022-03-23T18:45:30Z

@flash1293 about the references to icon position, it happens because the helpers are shared between reference lines and annotations so for reference lines it's still needed.
Copying styles from the last one in the list/ last one edited - what do you think about it @gvnmagni @MichaelMarcialis? Nevertheless, I'd address it in a separate PR.

The concept itself is really interesting, the only concern I have is that we have to find a proper way to explain what's happening to the user and I'm not sure there is a simple way to do that, I'll hear what Michael has to say

Yeah, my first instinct here is to agree with @gvnmagni. The concept is interesting, but I worry that the implementation would be non-obvious and potentially frustrating if that is not the desired behavior. I think some exploration can and should be done separately in terms of 1) users being able to establish style defaults for various aspects of Lens (including annotations) and 2) possibly consider the possibility of being able to bulk style/edit certain dimension items. Thoughts?

flash1293 · 2022-03-23T18:57:32Z

Yeah, no worries, definitely something to look into later, I just kind of expected this to happen that’s why I wanted to share.

kibana-ci · 2022-03-23T21:07:19Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`eventAnnotation`	-	11	+11
`lens`	758	784	+26
total			+37

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`eventAnnotation`	-	49	+49
`lens`	356	363	+7
total			+56

Async chunks

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

id	before	after	diff
`eventAnnotation`	-	661.0B	+661.0B
`lens`	1.1MB	1.1MB	+19.3KB
total			+20.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`eventAnnotation`	-	3	+3
`lens`	43	44	+1
total			+4

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`eventAnnotation`	-	5.6KB	+5.6KB
`lens`	44.2KB	44.9KB	+713.0B
total			+6.3KB

Unknown metric groups

API count

id	before	after	diff
`eventAnnotation`	-	49	+49
`lens`	429	438	+9
total			+58

async chunk count

id	before	after	diff
`eventAnnotation`	-	1	+1

ESLint disabled in files

id	before	after	diff
`eventAnnotation`	-	1	+1

Total ESLint disabled count

id	before	after	diff
`eventAnnotation`	-	1	+1

History

💚 Build #33112 succeeded 53cc632
💚 Build #32964 succeeded b8f327b
💚 Build #32921 succeeded 94484dc
💔 Build #32831 failed b9271ee
💔 Build #32806 failed 32eb53de3fd60402833d556ac23676f4d0b6cd58
💔 Build #32721 failed 8be973ed1d71927f91cf8e494c77e0fdd808ba73

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

mbondyra force-pushed the lens/annotation_service branch 2 times, most recently from 1f7c9a1 to 021117c Compare March 10, 2022 13:31

mbondyra added ci:deploy-cloud and removed ci:deploy-cloud labels Mar 10, 2022

mbondyra force-pushed the lens/annotation_service branch from e1906c0 to 526cad1 Compare March 14, 2022 10:01

elastic deleted a comment from kibana-ci Mar 14, 2022

mbondyra force-pushed the lens/annotation_service branch 17 times, most recently from 9d2f6fc to d624222 Compare March 17, 2022 20:08

stratoula mentioned this pull request Mar 18, 2022

[XY] xyVis and layeredXyVis layers. #128056

Closed

7 tasks

Kuznietsov mentioned this pull request Mar 18, 2022

[XY] Expression chart. #127150

Merged

2 tasks

mbondyra force-pushed the lens/annotation_service branch 3 times, most recently from 430816b to 63f804e Compare March 18, 2022 16:46

alexwizp reviewed Mar 23, 2022

View reviewed changes

src/plugins/event_annotation/public/plugin.ts Outdated Show resolved Hide resolved

alexwizp reviewed Mar 23, 2022

View reviewed changes

mbondyra added 2 commits March 23, 2022 11:10

refactor empty button

115aca7

CR

2f04298

alexwizp reviewed Mar 23, 2022

View reviewed changes

x-pack/plugins/lens/public/xy_visualization/annotations/config_panel/index.tsx Show resolved Hide resolved

alexwizp reviewed Mar 23, 2022

View reviewed changes

x-pack/plugins/lens/public/xy_visualization/annotations/helpers.tsx Outdated Show resolved Hide resolved

date cr

b8f327b

flash1293 reviewed Mar 23, 2022

View reviewed changes

nreese reviewed Mar 23, 2022

View reviewed changes

x-pack/plugins/maps/public/lens/choropleth_chart/region_key_editor.tsx Outdated Show resolved Hide resolved

remove DimensionEditorSection

0771829

mbondyra force-pushed the lens/annotation_service branch from 19dcb3d to 0771829 Compare March 23, 2022 15:41

flash1293 approved these changes Mar 23, 2022

View reviewed changes

mbondyra added 2 commits March 23, 2022 16:45

Merge commit 'c55bb917fab509df7922ac727d4246514dbd7a59' into lens/ann…

dea3181

…otation_service

change to emptyShade for traingle fill

028886a

nreese approved these changes Mar 23, 2022

View reviewed changes

stratoula approved these changes Mar 23, 2022

View reviewed changes

mbondyra commented Mar 23, 2022

View reviewed changes

x-pack/plugins/lens/public/app_plugin/app.scss Outdated Show resolved Hide resolved

Update x-pack/plugins/lens/public/app_plugin/app.scss

53cc632

Merge branch 'main' into lens/annotation_service

6c34ba6

mbondyra merged commit 2f9e6ee into elastic:main Mar 23, 2022

mbondyra added backport:skip This commit does not require backporting and removed ci:deploy-cloud labels Mar 23, 2022

KOTungseth mentioned this pull request Mar 30, 2022

Adds the Lens 8.2.0 docs #128986

Merged

mbondyra deleted the lens/annotation_service branch April 4, 2022 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Manual Annotations #126456

[Lens] Manual Annotations #126456

mbondyra commented Feb 28, 2022 •

edited

Loading

mbondyra commented Mar 10, 2022

alexwizp Mar 23, 2022

mbondyra Mar 23, 2022

markov00 Mar 23, 2022 •

edited

Loading

mbondyra Mar 23, 2022

flash1293 Mar 23, 2022 •

edited

Loading

MichaelMarcialis Mar 23, 2022

alexwizp Mar 23, 2022

mbondyra Mar 23, 2022

flash1293 left a comment •

edited

Loading

mbondyra commented Mar 23, 2022

gvnmagni commented Mar 23, 2022

flash1293 left a comment

nreese left a comment

stratoula left a comment

MichaelMarcialis commented Mar 23, 2022

flash1293 commented Mar 23, 2022

kibana-ci commented Mar 23, 2022

API count

async chunk count

ESLint disabled in files

Total ESLint disabled count

[Lens] Manual Annotations #126456

[Lens] Manual Annotations #126456

Conversation

mbondyra commented Feb 28, 2022 • edited Loading

Summary

Event annotations service

mbondyra commented Mar 10, 2022

alexwizp Mar 23, 2022

Choose a reason for hiding this comment

mbondyra Mar 23, 2022

Choose a reason for hiding this comment

markov00 Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

mbondyra Mar 23, 2022

Choose a reason for hiding this comment

flash1293 Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

MichaelMarcialis Mar 23, 2022

Choose a reason for hiding this comment

alexwizp Mar 23, 2022

Choose a reason for hiding this comment

mbondyra Mar 23, 2022

Choose a reason for hiding this comment

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

mbondyra commented Mar 23, 2022

gvnmagni commented Mar 23, 2022

flash1293 left a comment

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

MichaelMarcialis commented Mar 23, 2022

flash1293 commented Mar 23, 2022

kibana-ci commented Mar 23, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled in files

Total ESLint disabled count

History

mbondyra commented Feb 28, 2022 •

edited

Loading

markov00 Mar 23, 2022 •

edited

Loading

flash1293 Mar 23, 2022 •

edited

Loading

flash1293 left a comment •

edited

Loading