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

Adding a token component. #1270

Merged
merged 20 commits into from
Nov 6, 2018
Merged

Conversation

daveyholler
Copy link
Contributor

@daveyholler daveyholler commented Oct 30, 2018

Currently used primarily for code-based search results.

Summary

Creating a Token component for search result icons.

screen shot 2018-10-29 at 7 27 19 pm

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

Currently used primarily for code-based search results.
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.

Visuals are all good and matches the aesthetic well.

Let's go over the code in a zoom session (maybe @cchaos can join too). Right now there's not much you can do with the props on EuiToken itself other than set a size, since the coloring is hard coded and doesn't change per theme. If that's a hard rule then these would likely be better off as pure SVGs, but likely there's some cooler stuff we can do to utilize the theming and make this stuff a bit more flexible.

Can also walk you through some general Sass stuff. All of it's pretty easy.

@cchaos
Copy link
Contributor

cchaos commented Oct 30, 2018

I can certainly join in a code review sess.

At first glance, I agree that the visuals do align with the rest of EUI, though I wonder if the border is necessary? It takes my brain a while to discard the border and find the icon since the spacing between the border and icon is so condensed. However, it may come down to context. Could you add an example of usage?

Do we think we would also align the look of these with the query auto-completer (at some point)?
screen shot 2018-10-30 at 10 41 06 am

@snide
Copy link
Contributor

snide commented Oct 30, 2018 via email

daveyholler and others added 3 commits October 30, 2018 16:37
The `EuiToken` component now supports using any `EuiIcon` as it's glyph. Colors, shapes, sizes, borders, and solid colors can be set with props.
@snide
Copy link
Contributor

snide commented Oct 31, 2018

@daveyholler Here's a PR with some rearranging that should give you some more flexibility. daveyholler#1

@cchaos
Copy link
Contributor

cchaos commented Oct 31, 2018

@daveyholler I was looking at the contrast levels for your deterministic ones and noticed some don't pass AA levels. We have a SASS function called highContrastTextColor that will shade or tint a foreground color based on a background color until it meets AA levels. I think this would help you a lot. For instance:

Light theme color adjustment:

Changing the code to read:

@each $tintName, $tintValue in $tokenTypes {
  .euiToken--#{$tintName} {
    box-shadow: 0 0 0 1px $tintValue;

    $backgroundColor: tintOrShade($tintValue, 70%, 70%);
    background-color: $backgroundColor;

    [...]

    svg {
      fill: makeHighContrastColor($tintValue, $backgroundColor);
    }
  }
}

Will adjust that svg fill just enough to pass AA contrast levels:

However

Right now the color function doesn't account for background colors that might stay light in dark mode. (I'll have a PR fix for that soon). So in the above code, I also altered the background to go dark instead of light in dark mode.

But I know you worked on creating a color theme that's light and works in both light and dark mode. So if you want to keep with that same color theme in dark mode, you'll just need to wait for my PR. And then you'll be able to change

$backgroundColor: tintOrShade($tintValue, 70%, 70%);

back to

$backgroundColor: tint($tintValue, 70%);

DO NOT try to use the color function without adjusting the background color to use tintOrShade, it will break the compiler.... That's how I found out we had an issue 👎

Here are the examples of the options above in dark mode:

@snide
Copy link
Contributor

snide commented Oct 31, 2018

@cchaos That reversed dark theme looks rad.

@daveyholler
Copy link
Contributor Author

@cchaos This is legitimately awesome. I'll throw the SASS function in. I actually dig the inverted theme in dark mode. You rock! Thanks!

daveyholler and others added 6 commits October 31, 2018 11:29
* Integrating @snide `displayOptions` method for setting props
* Providing a `hideBorder` option
* Including a `large` and `hideBorder` example in the docs
* Removing duplicate SVG files
* Renaming a few files for clarity
* Removing the tokens from the general icon example list
* Wordsmithing the docs for clarity
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.

The final result looks good. I just have few suggestions/comments.

It would probably be nice to also go ahead and add a TS defs file. @chandlerprall could help with that.

CHANGELOG.md Outdated Show resolved Hide resolved
src/components/icon/assets/tokens/tokenFile.svg Outdated Show resolved Hide resolved
src/components/icon/assets/tokens/tokenNumber.svg Outdated Show resolved Hide resolved
src/components/icon/assets/tokens/tokenString.svg Outdated Show resolved Hide resolved
src-docs/src/views/icon/tokens.js Show resolved Hide resolved
src/components/token/index.js Outdated Show resolved Hide resolved
src/components/token/token.js Show resolved Hide resolved
src/components/token/token.js Outdated Show resolved Hide resolved
src/components/token/token.test.js Show resolved Hide resolved
@daveyholler
Copy link
Contributor Author

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.

Changes LGTM!

src/components/token/index.d.ts Show resolved Hide resolved
src/components/token/index.d.ts Outdated Show resolved Hide resolved
src/components/token/index.d.ts Outdated Show resolved Hide resolved
src/components/token/token.js Outdated Show resolved Hide resolved
src/components/token/index.d.ts Outdated Show resolved Hide resolved
chandlerprall and others added 5 commits November 6, 2018 11:41
Co-Authored-By: daveyholler <daveyholler@gmail.com>
Co-Authored-By: daveyholler <daveyholler@gmail.com>
Co-Authored-By: daveyholler <daveyholler@gmail.com>
@daveyholler daveyholler merged commit b6fdd09 into elastic:master Nov 6, 2018
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.

4 participants