EuiCards #385

cchaos · 2018-02-09T21:03:29Z

First pass at creating a card component.

A basic card requires a title and description.
Cards can either take an Icon in node form or Image url
Card footers can be any combination of nodes
Footers will stick to the bottom visually so all footers in a grouping of cards will line up
If a card's onClick attribute exists, it will apply the new isHoverable prop to the containing EuiPanel for hover effects

To do:

Clickable cards are focusable

snide

Chatted a bit over slack.

Needs focus states to work (just needs accessible wrapper for onclick).
Should add a custom validator similar to our buttons, so that you can't use icon AND image together.

snide · 2018-02-09T23:02:08Z

src/components/card/_card.scss

+}
+
+.euiCard__image {
+  .euiIcon {


Should add a class for this instead of overwriting through nests.

snide · 2018-02-09T23:02:15Z

src/components/card/_card.scss

+    margin-bottom: $euiCardSpacing;
+  }
+
+  .euiImage {


snide · 2018-02-09T23:04:00Z

src/components/panel/_panel.scss

+    &:hover,
+    &:focus {
+      transform: translateY(-2px);
+      @include euiSlightShadowHover;


Prolly just move this a line up.

snide · 2018-02-09T23:05:07Z

src/components/card/card.js

+  let imageNode;
+  if (image) {
+    imageNode = (
+      <EuiImage url={image} alt="" />


Maybe pass the title in the alt?

I left the alt blank here on purpose since the image should not be representing content or describing something in this instance. It's strictly a visual aid to the card's title. Adding the card title to the alt will make the screen ready read the card's title twice.

snide · 2018-02-09T23:06:25Z

src/components/panel/panel.js

 };

 EuiPanel.defaultProps = {
  paddingSize: 'm',
  hasShadow: false,
  grow: true,
+  isHoverable: false,


Maybe add some auto-gen prop docs to this to explain it.

We should probably also document this on the EuiPanel page as well. Doubtful it would be used outside of a card, but prolly a good idea.

snide

Blargh. I suck at GH today.

cchaos · 2018-02-12T23:50:42Z

Address comments and open for final review.

cjcenizal

This is fantastic! I just had one suggestion.

cjcenizal · 2018-02-13T00:17:32Z

src/components/panel/panel.js

@@ -50,10 +52,12 @@ EuiPanel.propTypes = {
  paddingSize: PropTypes.oneOf(SIZES),
  grow: PropTypes.bool,
  panelRef: PropTypes.func,
+  isHoverable: PropTypes.bool,


Unless we can think of a situation where we want this behavior to be uncoupled from an onClick callback, then I think it's worth just renaming this to be onClick: PropTypes.func. WDYT?

So we should pass the onClick from EuiCard onto EuiPanel?

I can think of situations where you wouldn't want the card to bounce, but still want onClick. Common one would be W I D E P A N E L that is skinny vertically.

Hey, DO THIS THING, IM A BIG CALLOUT at the top.

Putting the hover on the card when it's such a large element is usually something you wouldn't want. Normally works better for bite-sized repeatable elements (which is the common scenario).

But you're right it's not gonna happen very often.

I think I know what you're talking about. I've seen that pattern on the Bootstrap site. It's pretty neat though at first I didn't realize it was clickable. Maybe that pattern should require a button inside the panel instead? Or a new kind of component which has a noticeable hover state, like a background change?

I think that's better for documenting patterns though. Large panels like jumbotrons should have buttons as actions not be an action itself.

snide

Yay. Cards.

cjcenizal

🌞

… if it exists Also making Card’s text left aligned by default.

cchaos · 2018-02-13T02:56:16Z

@cjcenizal & @snide Can you take one more look at this (mainly just that last commit).

On @cjcenizal's request, I passed the onClick handler to the EuiPanel and changed the outer element to a <button> if it has onClick. I had to add some more styles to Panel in case of this since <button>'s don't have the same inherent display properties as <div>.

Also, I realized there may be good cases for cards that have their content left aligned vs centered, so I gave it a isCentered prop. Though now that I think on it, I'm going to change that to alignment and have it take 'left', 'center', or 'right' and default to 'center'.

…defaulting to ‘right’

snide · 2018-02-13T03:10:55Z

@cchaos Looks good to me. Make your prop "textAlign" instead of "alignment" just so it's a little more specific to what it does.

I think the PanelTag bit works fine here.

cjcenizal

Looks awesome!

cchaos added the work in progress label Feb 9, 2018

cchaos requested a review from snide February 9, 2018 21:03

cchaos force-pushed the cards branch from f99e1cd to b20a0a2 Compare February 9, 2018 21:08

snide suggested changes Feb 9, 2018

View reviewed changes

snide reviewed Feb 9, 2018

View reviewed changes

src/components/card/_card.scss Outdated

margin-bottom: $euiCardSpacing;

}

.euiImage {

Copy link

Contributor

snide Feb 9, 2018

Choose a reason for hiding this comment

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

Same here.

snide reviewed Feb 9, 2018

View reviewed changes

cchaos force-pushed the cards branch from 496f9ae to 4670a69 Compare February 12, 2018 23:49

cchaos added feature request and removed work in progress labels Feb 12, 2018

cchaos requested a review from cjcenizal February 12, 2018 23:50

cjcenizal reviewed Feb 13, 2018

View reviewed changes

snide approved these changes Feb 13, 2018

View reviewed changes

cjcenizal approved these changes Feb 13, 2018

View reviewed changes

cchaos added 9 commits February 12, 2018 21:07

Initial EuiCard files and content

567da79

snaps

2b32be7

First pass at EuiCard

ec6e434

A little more separation of icon from top of card

1295517

Adding focusablility to card if onClick supplied

beda3c4

Adding doc example of isHoverable panel.

b62ebec

Better classNames for Icon, Image and allowing for both

5ad5bee

Updating docs

0155035

changelog

ad638af

cchaos force-pushed the cards branch from 4670a69 to ad638af Compare February 13, 2018 02:08

Passing onClick to Panel and changing Panel’s outer element to button…

25b767b

… if it exists Also making Card’s text left aligned by default.

Allowing card’s alignment to be one of ‘left’, ‘center’, or ‘right’, …

90b2dec

…defaulting to ‘right’

‘alignment’ -> ‘textAlgin’

918dc33

cjcenizal approved these changes Feb 13, 2018

View reviewed changes

Merge branch 'master' into cards

d610377

cchaos merged commit ebfe6fd into elastic:master Feb 13, 2018

cchaos deleted the cards branch February 13, 2018 14:58

snide mentioned this pull request Feb 13, 2018

Near term EUI roadmap #108

Closed

51 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EuiCards #385

EuiCards #385

cchaos commented Feb 9, 2018 •

edited

Loading

snide left a comment

snide Feb 9, 2018

snide Feb 9, 2018

snide Feb 9, 2018

snide Feb 9, 2018

cchaos Feb 12, 2018

snide Feb 9, 2018

snide Feb 9, 2018

cchaos Feb 12, 2018

snide left a comment •

edited

Loading

cchaos commented Feb 12, 2018

cjcenizal left a comment

cjcenizal Feb 13, 2018

cchaos Feb 13, 2018

snide Feb 13, 2018 •

edited

Loading

cjcenizal Feb 13, 2018 •

edited

Loading

cchaos Feb 13, 2018

snide left a comment

cjcenizal left a comment

cchaos commented Feb 13, 2018 •

edited

Loading

snide commented Feb 13, 2018

cjcenizal left a comment

EuiCards #385

EuiCards #385

Conversation

cchaos commented Feb 9, 2018 • edited Loading

snide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide left a comment • edited Loading

Choose a reason for hiding this comment

cchaos commented Feb 12, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

cjcenizal Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

cchaos commented Feb 13, 2018 • edited Loading

snide commented Feb 13, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

cchaos commented Feb 9, 2018 •

edited

Loading

snide left a comment •

edited

Loading

snide Feb 13, 2018 •

edited

Loading

cjcenizal Feb 13, 2018 •

edited

Loading

cchaos commented Feb 13, 2018 •

edited

Loading