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

Add image glyph to EuiIcon #2870

Merged
merged 5 commits into from
Feb 19, 2020
Merged

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Feb 19, 2020

Summary

As requested in #2844

image

image

Note: Some files I didn't directly edit have been updated. I'm guessing this is a result of running yarn compile-icons

Fixes #2844

Checklist

- [ ] Check against all themes for compatibility 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

@andreadelrio andreadelrio changed the title Add image glyph to EuiIcon [WIP] Add image glyph to EuiIcon Feb 19, 2020
@andreadelrio andreadelrio changed the title [WIP] Add image glyph to EuiIcon Add image glyph to EuiIcon Feb 19, 2020
@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2020

@andreadelrio Can you share a screenshot with the pixel grid?

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2020

Also, when I look at the screenshot of the icon next to the others, this one feels a bit too bold. Usually we reserve filled areas for smaller portions like the folded corner of a page or a small circle with a plus. What if the mountains were outlines that reached all the way to the frame edges and just left the sun filled in?

@andreadelrio
Copy link
Contributor Author

@andreadelrio Can you share a screenshot with the pixel grid?

image

Also, when I look at the screenshot of the icon next to the others, this one feels a bit too bold. Usually we reserve filled areas for smaller portions like the folded corner of a page or a small circle with a plus. What if the mountains were outlines that reached all the way to the frame edges and just left the sun filled in?

I was playing a bit with this idea. See:

image

image

I also noticed the version with filled mountains felt a bit off but couldn't quite put my finger on why...I think it looks a bit too "crowded" specially when used at 16px size .

@andreadelrio
Copy link
Contributor Author

After taking feedback, the new icon looks like this:

image

image

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.

I think the outline for the mountains works a lot better. 🎉 👍

@andreadelrio andreadelrio merged commit eef8917 into elastic:master Feb 19, 2020
@ryankeairns
Copy link
Contributor

Thanks @andreadelrio !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[icons] Icon request: image
3 participants