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

Adjusting EuiToken for use as ES field types #2758

Merged
merged 19 commits into from
Feb 5, 2020
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jan 14, 2020

Live build: https://cchaos-current-pr.netlify.com/#/display/icons

With the unfortunate demise of Code, we were left with the opportunity to reuse EuiToken for our need to provide a service/icon for representing ES field type (which are showcased all over Kibana and in different ways at the moment).

This PR, causes some breaking changes to EuiToken to suit these new needs, but without completely butchering the original intent (hopefully, @daveyholler).

I will start by explaining the changes I've made and then demonstrate use cases.

Changes (visual and code-breaking)

Prop changes

Before, in order to change the display of the token you had to pass an object called displayOptions which supported the keys of color, shape, fill, and hideBorder. Now, you can pass these keys directly to the EuiToken component as props.

Screen Shot 2020-01-14 at 15 24 17 PM

More on the actual prop value changes below.

Color

Visual

When we test using the Tokens as they were in Discover, the colors were very bright and were a bit too Crayola. This PR subdues the background by fading it even more and decreasing the contrast of the border.

Screen Shot 2020-01-14 at 15 27 32 PM

Entire list of icon before and after below.

Code

The originally accepted colors for tokens was an array of enums from tokenTint01 to tokenTint10 that were mapped to colors in a SASS map. It used a combination of brand palette, vis palette, and custom hex values. I was able to remap these colors to use all vis palette colors plus the addition of a 'gray'.

To reduce cognitive load on the consumer, however, I removed the idea that they had to remember or look for the colors of these custom tokenTint colors. Instead, they can use the vis palette naming scheme that has already been established and be found on the Colors guideline page (euiColorVis0 to euiColorVis9).

Screen Shot 2020-01-14 at 15 36 46 PM

Further, EuiToken now allows consumers to pass any valid color string as the color property. If it is not one of the vis named colors, it will simply create the token version that has the fully colored background.

Screen Shot 2020-01-14 at 15 40 06 PM

Fill

Previously fill was a simple boolean that, when true, would turn the normally faded background color to a non-faded (fully opaque) background color. Now, this prop is an enum supporting:

  • light for the faded version
  • dark for the non-faded version
  • none for no background at all

Screen Shot 2020-01-14 at 15 59 18 PM

Fixes

When using the fill=true it changed the icon's fill from being the color supplied to a hard-coded white. This could cause issues with contrast, so instead, the new fill=dark property, uses lightOrDarkText to determine if the text should be white or black.

image

hideBorder

This property has been completely removed in favor of the simple fill properties above.

However, the original border was an outset box-shadow that would turn the normally 16x16 token into a visually 18x18 token. The main issue I ran into was that this would cause the icon to get cut off if it's parent had overflow: hidden.

Example:
Screen Shot 2020-01-13 at 14 55 23 PM

So instead I changed the box shadow to be inset, which maintains the visual size and fixes this problem. Though maybe the spacing between icon and and border are not as ideal.

Final changes of the original set

Screen-Shot-2020-01-10-at-17 54 01-PM
Only a few colors values for the token types have changed but overall kept the original design.


More icons

EuiToken uses a subset of icons that begin with token like tokenString. These are essentially 12x12 pixel versions of the originals. To maintain pixel alignment at these smaller sizes the glyphs themselves were tweaked. Therefore, I also had to add token (12x12) versions of:

  • tokenAlias
  • tokenDate
  • tokenGeo
  • tokenIP
  • tokenNested
  • tokenRange
  • tokenShape

These are the ES field type tokens created for Kibana:

Screen Shot 2020-01-14 at 16 14 11 PM

Example usage in Discover

image

I also updated EuiDataGridShchema to extend EuiToken instead of simply EuiIcon

Screen Shot 2020-01-11 at 08 50 35 AM

Checklist

Will do all the unchecked list items after review in case there are major changes.

  • 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

cchaos added 13 commits January 10, 2020 16:37
…ette

Slight changes to these colors:
- Neon pink became light pink
- Primary is no longer used but uses the blue from vis palette instead
…ph names

Cleans up Token code
Makes background color more subtle
- Fill now takes ‘light’, ‘dark’, or ‘none’
- No more `hideBorder`, comes with ‘light’ fill
- Had to create a couple mixins to support changing the tint color from the graphic to behindText variant
- $tokenTypes -> $euiTokenTypes
- Now restricting to only 10 colors
   - removed 12 (hot pink)
   - changed usage of 11 (hot green) to 00 (vis green)
- Added $euiPaletteColorBlindKeys for finding if key exists in map
…me names for the vis palette (euiColorVis)

Plus on extra called “gray” that gets appended via SASS map-merge
- Allowing passing of a custom color value (but only works with none or dark fill)
@cchaos cchaos marked this pull request as ready for review January 14, 2020 22:21
@ryankeairns ryankeairns self-requested a review January 15, 2020 14:00
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

This looks great, my feedback is largely suggestions so marking as Approved.

Side note
The files weren't touched directly by this PR, but the tokens in the Tree View component seem a little smaller than they were previously, perhaps on the verge of too small.

src-docs/src/views/icon/tokens.js Outdated Show resolved Hide resolved
src-docs/src/views/icon/tokens.js Show resolved Hide resolved
src/components/token/token_map.ts Show resolved Hide resolved
src/components/token/_token.scss Outdated Show resolved Hide resolved
src/components/token/token.tsx Show resolved Hide resolved
src/components/token/token.tsx Show resolved Hide resolved
@chandlerprall chandlerprall self-requested a review January 15, 2020 17:23
src/components/token/token.tsx Outdated Show resolved Hide resolved
src/components/token/token.tsx Show resolved Hide resolved
src/components/token/token.tsx Outdated Show resolved Hide resolved
src/components/token/token.tsx Outdated Show resolved Hide resolved
@cchaos cchaos requested a review from chandlerprall February 4, 2020 16:26
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled & tested locally

@cchaos
Copy link
Contributor Author

cchaos commented Feb 5, 2020

Jenkins, test this

@cchaos cchaos merged commit 76d4435 into elastic:master Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants