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 euiStat #1146

Merged
merged 4 commits into from
Aug 29, 2018
Merged

Add euiStat #1146

merged 4 commits into from
Aug 29, 2018

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Aug 27, 2018

Adds new EuiStat metric component to EUI.
Closes #559

EuiStat can be used to display prominent text or number values. It consists of title and description elements with several visual styling properties as seen below.

stat-screenshot

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.

Yay. First component. Looks excellent.

Works as advertised and I think it's a generic enough design pattern it can be used in lots of places. I left some comments on the code, but they are very minor.

For a later PR, I bet @cchaos or I can figure out a way to automatically scale the size down if there isn't enough room, but the setup you've got leaves us the room to do that pretty easily.

Nice work.

const colorToClassNameMap = {
default: null,
dark: 'euiStat--dark',
full: 'euiStat--full',
Copy link
Contributor

Choose a reason for hiding this comment

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

You have a full and a default. Default doesn't currently set styling of any kind. You might want to remove full and apply it's coloring to default. Because default is the titleColor default value, it'll get that title coloring (which design wize seems appropriate in most cases) applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Writing that out it reads like a tongue twister... so here's what I mean.

default: null,

// remove `euiStat--full,

Then in your .scss

.euiStat__title
  color: $euiColorFullShade;

  @each $name, $color in $titleColors {
    &--#{$name} {
      color: $color;
    }
  }

See the below comment on the JS side to see why it's good to apply the modifier to the title and not the stat itself.


const colorToClassNameMap = {
default: null,
dark: 'euiStat--dark',
Copy link
Contributor

Choose a reason for hiding this comment

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

To follow pattern with our other components, I'd define this as subdued rather than dark


const classes = classNames(
'euiStat',
colorToClassNameMap[titleColor],
Copy link
Contributor

@snide snide Aug 28, 2018

Choose a reason for hiding this comment

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

This color isn't for the color of the whole stat block, just the title.

You might want two class constants to better make that distinction.

  const classes = classNames(
    'euiStat',
    textAlignToClassNameMap[textAlign],
    className,
  );

  const titleClasses = classNames(
   'euiStat__title`,
    colorToClassNameMap[titleColor],
  );

Then on your titleDisplay below you can do this...

<EuiTitle size={titleSize} className={titleClasses}>


if (reverse) {
statDisplay = (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

When we reviewed over zoom I forgot to explain a way to not use empty divs. When you run into situations like this where you want to return multiple elements in React, you can use a <Fragment>

https://reactjs.org/docs/fragments.html

First at the top of your JS file you want to make sure to import it.

import React, { Fragment } from 'react';

Then simply replace the div with the fragment.

<Fragment>
  {titleDisplay}
  {descriptionDisplay}
</Fragment>

Now the DOM won't render extra cruft.

@ryankeairns
Copy link
Contributor Author

Thanks for the feedback @snide , changes made, build passed!

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.

All good. Nice work!

@ryankeairns ryankeairns merged commit e1184d8 into elastic:master Aug 29, 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.

2 participants