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

[ICON] Adding annotation icon #2691

Merged
merged 5 commits into from
Dec 19, 2019
Merged

[ICON] Adding annotation icon #2691

merged 5 commits into from
Dec 19, 2019

Conversation

formgeist
Copy link
Contributor

Summary

Adding a new annotation glyph to be used as a legend icon for annotations in Observability/APM.

Screenshot 2019-12-18 at 16 32 21Screenshot 2019-12-18 at 16 32 29

Example of use

Slice 1

Checklist

  • Check against all themes for compatability in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@formgeist formgeist self-assigned this Dec 18, 2019
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

@formgeist I'm really glad you showed me an example of using it with charts. I decided to check if (using Elastic-charts) your mock would actually work. My main question was, since the icon isn't completely centered in the 16x16 square (it's shifted right to align to the pixel grid), would it align with the full line created by the annotation?

I added an annotation to the charts example and can see that, in fact, it doesn't actually line up. It is one pixel too far to the right.

Screen Shot 2019-12-18 at 14 48 49 PM

Consider shifting the icon left of center instead of right, and then I think it should line up with the annotation line. We also err on shifting off-center icons left anyway so it's a win-win.

@formgeist
Copy link
Contributor Author

@cchaos I can see how that would be beneficial to use, so I'll tweak the icon to be able to fit those use cases as well. We were specifically going to use the icon for the legend only in this iteration, and create a custom circle element to the annotation in the actual charts, since we've implemented our own React-vis charts (since we haven't switched over to Elastic Charts just yet). But your suggestion would mean we could use the icon in there too and switch easily to the annotation option in Elastic Charts. Thanks for the feedback and for doing this test 👍

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Hmm, whelp my theory was wrong. Looks like the Elastic-Chart's annotation is rendering in a sub-pixel (dang retina screen).

Screen Shot 2019-12-18 at 15 39 36 PM

But like I said, we usually push off-center icons left instead of right anyway so no harm. It all looks good to me so long as you also meant to increase the size of the dot in your last iteration?

@formgeist
Copy link
Contributor Author

Oh, bummer... I'm so used to it too that I rarely check 🤓Yeah, I bumped the size of the dot because I thought it balanced better as an icon on the annotation line. Either way, we can see how it works on our end otherwise we might use a normal circle shape for the annotation line and use the glyph simply for the legends.

@formgeist formgeist merged commit 00e51cf into elastic:master Dec 19, 2019
@formgeist formgeist deleted the annotation-icon branch December 19, 2019 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants