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 card graphics #1338

Merged
merged 9 commits into from
Dec 3, 2018
Merged

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Nov 30, 2018

Fixes #1334

Summary

In preparation for Kibana home page changes, this PR add an optional svg graphic to cards.
An example card using this prop has been intentionally left off the EuiCard docs page as it should only be used in select cases.

Light theme

screenshot 2018-11-30 13 14 50

Dark theme

screenshot 2018-11-30 13 12 31

Kibana home sample

screenshot 2018-11-30 14 14 19

Checklist

  • This was checked in mobile
  • This was checked in IE11 - So far unable to load EUI in IE :/
  • 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
  • This required updates to Framer X components

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.

What do you think about moving the SVG-specific code out into a new component (card_svg.js ?) and letting that component be passed in to EuiCard? It will help bundle size in the future when we have efficient splitting, and is more flexible in what can be used at the footer of the card.

src/components/card/card.js Outdated Show resolved Hide resolved
@ryankeairns
Copy link
Contributor Author

@chandlerprall This PR is ready for a fresh review. I've moved the svg to its own component, as suggested.

@ryankeairns
Copy link
Contributor Author

@chandlerprall I've made the requested changes for future code splitting concerns. Ready for another look, thanks.

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.

Looking good. Had a few suggestions around naming and SASS.

Also, do you think the graphic should crop or scale?

Examples of it now (cropping):

Example of scaling it to always be 100% the width of the card (though I cropped it at 300px wide too):

src/components/card/card.js Show resolved Hide resolved
src/components/card/card_bg.js Outdated Show resolved Hide resolved
src/components/card/_card.scss Outdated Show resolved Hide resolved
src/components/card/_card.scss Outdated Show resolved Hide resolved
src/components/card/_card.scss Outdated Show resolved Hide resolved
src/components/card/_card.scss Outdated Show resolved Hide resolved
src/components/card/card.js Show resolved Hide resolved
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.

Code LGTM

@cchaos
Copy link
Contributor

cchaos commented Dec 3, 2018

One more thought is that I really like how in dark mode, the gray shape has a defined shape.

image

Can you do the same for light mode?

image

I know it's hard to get one value looking great for both....

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Dec 3, 2018

@cchaos This is ready for a fresh review. I've also tweaked the colors a bit to better see the second wave on the light theme. Ideally, we would use different color values on that particular path in the svg depending upon the active theme, but Dave noted that while there is a ticket for such a detection mechanism, it does not yet exist. I think, after this latest tweaking, it's a good balance between the two themes for now.

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.

Now I'm just being nit picky, so feel free to disregard. But what do you think about changing the light shape to just be a solid color?

<path fill="#bbb" fillOpacity=".13" fillRule="evenodd" d={graphicSVGPathLight} />

src/components/card/card_graphic.js Outdated Show resolved Hide resolved
@ryankeairns ryankeairns merged commit daaf12e into elastic:master Dec 3, 2018
@snide snide mentioned this pull request Dec 7, 2018
3 tasks
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.

3 participants