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 tokenCurrency to EuiIcon #2398

Merged
merged 1 commit into from
Oct 3, 2019

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Oct 3, 2019

Fixes #2395

Summary

Adds generic 'tokenCurrencyglyph toEuiIcon`. The shape is a universal symbol for generic currency.

Preview

Screenshot 2019-10-03 10 49 54

Screenshot 2019-10-03 10 50 53

Checklist

  • Checked in dark mode
  • 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

@ryankeairns ryankeairns marked this pull request as ready for review October 3, 2019 15:54
@ryankeairns ryankeairns requested a review from snide October 3, 2019 15:54
@ryankeairns
Copy link
Contributor Author

@snide One thing I've learned is that tokens always have a background color. You can change shape, drop the border, or do a solid fill, but you pick from 1 of 10 tints.

The question now is, do we push forward with tokens in your case and use a background, or do we move this currency glyph to the base icon set and you color it from there as Caroline did for Lens?

@snide
Copy link
Contributor

snide commented Oct 3, 2019

@ryankeairns I think it's OK to go with tokens. The color doesn't matter as long as they are somewhat qualitative. If we DO go in the tokens route, i think we'll need some tokens for things we have icons for. For example: bringing the globe over for Geo? Cal for Date, Link for URL...etc.

I leave the design call up to you to how you want to handle this stuff and implement whatever you think is best. You might want to check out lens and see what @cchaos is doing over there as well.

@daveyholler
Copy link
Contributor

FWIW, although tokens technically let you put any EuiIcon you want in there, some icons scale a little funny to the smaller size the token uses.

Basically just weighing in that duplication may occasionally be necessary across tokens and icons at the sizes we're currently using. 🙂

@ryankeairns
Copy link
Contributor Author

Taking that all into account, I think it would be best to use a colored icon (with no background color or shape), so I'm going to move this tokenCurrency icon to a base currency icon so that the data grid work can move forward.

That said, it does seem like we perhaps ought to be using tokens in instances such as this, however that component does not currently support the no-background result that I'm after here.

By moving this icon to the base set, it can still be used by EuiToken down the road. I'll also open a discussion issue about possibly adding this alternate no-background design to EuiToken or handling this in some other fashion (including how we move/duplicate existing icons from base to token and vice-versa).

@ryankeairns ryankeairns force-pushed the rk/datagrid-tokens branch 2 times, most recently from b67de6c to 013201b Compare October 3, 2019 18:44
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.

Works for me. Now that it's in the regular icon set, do we care that this is a heavier stroke? Your call. Safe to merge.

image

@ryankeairns
Copy link
Contributor Author

Since this a universally known icon, and it follows that design, I'm good with it.

Also, as Davey pointed out, you sometimes have to optimize for the token style which this does (for better or worse at the moment). The thicker style also seems to fit in alongside some of the other token-ish icons (see below), all of which may move and/or be optimized for each set.

Screenshot 2019-10-03 14 43 00

@ryankeairns ryankeairns merged commit d02dde5 into elastic:master Oct 3, 2019
snide pushed a commit to snide/eui that referenced this pull request Oct 10, 2019
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 for data schema
3 participants