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

Adds bug, flag, and heart icons. Updates alert icon. #1887

Merged
merged 4 commits into from
Apr 29, 2019

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Apr 25, 2019

Closes #1871

Summary

#1871 requests several icons:

  • bomb
  • bug
  • exclamation-circle
  • exclamation-triangle
  • fire
  • flag
  • heart

This PR adds the following news icons to EUI which we feel are most re-usable/common:

  • bug, flag, and heart

Screenshot 2019-04-25 16 32 13

Screenshot 2019-04-25 16 32 30

Screenshot 2019-04-25 16 32 39

For bomb and fire, I've created the svg icons and will open a separate PR on the Kibana side to add those. In the upcoming release of EUI, functionality will exist allowing you to use any icon with EuiIcon. We felt these two were better suited to live externally as we're also trying to keep our ever-expanding icon library manageable.

Screenshot 2019-04-25 16 53 33

For the exclamation-triangle, we recommend using the existing alert icon
Screenshot 2019-04-25 16 32 03

For the exclamation-circle, we recommend using the existing iInCircle icon
This better differentiates it from the exclamation-triangle icon while conveying a less severe, "informational", meaning like making a note.

Screenshot 2019-04-25 16 54 59


⚖️ Side note: I updated the svg for the alert icon since the right corner was a little askew
Here is what it looked like before
Screenshot 2019-04-25 16 24 26

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@ryankeairns ryankeairns requested review from cchaos and snide April 25, 2019 22:03
@ryankeairns ryankeairns marked this pull request as ready for review April 25, 2019 22:05
@ryankeairns
Copy link
Contributor Author

@snide Where should the two non-EUI icons (bomb and fire) be stored in the Kibana repo?

Globally available in one of these places...

src/legacy/ui/public/assets/images/
src/legacy/core_plugins/kibana/public/assets/

...or with the TSVB plugin code?

cc: @Avinar-24 @alexwizp

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

LGTM. The alert is a little softer, likely because you're not positioning it exactly on the grid (which is why it was probably off center originally). I could go either way on which is more annoying, so this is fine.

image

@ryankeairns
Copy link
Contributor Author

I don't see the fuzziness on my side, or at least it's not as noticeable. All I really did was pull in the right point a smidge, so I'm going to roll with this newer version.

Screenshot 2019-04-29 12 50 16

@cchaos
Copy link
Contributor

cchaos commented Apr 29, 2019

@ryankeairns The fuzzyness happens when not on a ultra-dense (retina) style monitor. You'd need an older monitor to test this the actual render, but to mitigate these from happening, aligning as much to the pixel grid as possible will ensure it's as crisp as possible.

The alert screenshot you showed (I'm guessing older is on the right?) does look odd. I'm not sure why the right bottom point of the triangle was further out.

I had to glance at the Sketch file to look at the pixel alignment and the alert is fine, I wish the exclamation point could be more inside the pixel line instead of straddling it like this:


As for the new icons they all look good except that the flag could use a nudge to the left:

@cchaos
Copy link
Contributor

cchaos commented Apr 29, 2019

Also for your recommendation:

For the exclamation-circle, we recommend using the existing iInCircle icon.

You should recommend that they just use the alert icon as well because they both use an exclamation. The iInCircle is more "information" while the alert is "warning".

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Apr 29, 2019

@cchaos Such is the life of a 1px outline-based icon set :)

Good catch on the flag! Initially, I was shaving off bytes by building these as lines as opposed to shapes, but that visually got away from our existing pattern of 1 and 2 pixel padding around the glyph itself. It also presented more challenges in handling the Sketch symbol color overrides, so I went with converting to outlines.

@ryankeairns
Copy link
Contributor Author

@cchaos

Also for your recommendation:

For the exclamation-circle, we recommend using the existing iInCircle icon.

You should recommend that they just use the alert icon as well because they both use an exclamation. The iInCircle is more "information" while the alert is "warning".

The thought was that they could differentiate the two better by using one as an informational-level warning and purposely not have two with exclamation points. I'll re-word that to state the options more clearly.

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.

Makes sense. Thx!

@ryankeairns
Copy link
Contributor Author

jenkins test this

@ryankeairns ryankeairns merged commit dd03a78 into elastic:master Apr 29, 2019
@ryankeairns
Copy link
Contributor Author

@Avinar-24 @alexwizp Attached are the SVG files for the two icons we've decided should not be included with EUI (as noted above). They can be added to Kibana globally, or to the plugin code as you see fit.

fire and bomb icons.zip

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.

[Icons] Need Icons for the EuiIcon component
3 participants