Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Lens] Manual Annotations #126456
[Lens] Manual Annotations #126456
Changes from 46 commits
472aa2e
422a231
308778e
6f9a9c9
7f2b283
723d191
af76be3
b97ba4c
df2d6d8
0d8edbd
d99ea65
fc451cb
512b09a
f83332c
465a8d1
b82c9f4
e9a8b75
fcfe987
ed3f858
437d464
653a226
3cca99a
b0504f7
91cf292
30276fe
7d0221a
21b32f8
0e3bd11
9b5c034
471d207
8a201b9
d4e3cf0
2a7f1f5
2a96147
a87c8a3
70ad1cf
f7e5c92
0c126c4
fd789e4
fc618c1
64f115c
75c4214
523af3e
b9271ee
94484dc
269e876
115aca7
2f04298
b8f327b
0771829
dea3181
028886a
53cc632
6c34ba6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I probably recommended not to fix the value at the time of import. Maybe
geDefaultAnnotationColor = () => euiLightVars.euiColorAccent;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it matter? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markov00 does it matter? Why is it concerning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.