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

More EuiCard props and added accessibility icon #2566

Merged
merged 8 commits into from
Nov 27, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Nov 26, 2019

After viewing the accessibility PR changes to EuiCard in Kibana, there was a particular usage that needed some extra customization from the EUI side.

Kibana implementation

Screen Shot 2019-11-26 at 12 58 55 PM

Added titleSize prop to EuiCard

This allows the shrinking of the title to better match a large size icon (vs a xl size icon). And in case the title is just too large beneath a particular heading.

I'm restricting to only s and xs (matches EuiTitle of the same sizing) so that the title can never be too large or too small compared to the description.

Screen Shot 2019-11-26 at 13 56 06 PM

Added display prop to EuiCard

The Kibana implementation uses EuiCard's within an EuiPanel which normall would be too many panels within panels. So in Kibana, they override the default EuiCard styles to remove the border and shadow.

I've added this as a property of EuiCard where the default is display="panel" but you can also set to display="plain" which, as assumed just removes the border and shadow.

Screen Shot 2019-11-26 at 12 58 39 PM

I am purposefully not creating a docs example for this as most cases should use the panel style.

Added accessibility glyph to EuiIcon

I was altering a call out within the docs that was mentioning accessibility and I really just wanted to add the icon. So I finally created one.

Screen Shot 2019-11-26 at 13 25 40 PM

Screen Shot 2019-11-26 at 13 34 11 PM

Screen Shot 2019-11-26 at 13 34 19 PM

Checklist

  • Checked in dark mode
  • 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

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.

A bunch of smalls, tried this out locally and it looks good!
I know I've run into the title size issue before and I really like the look of images in the "plain" display style.

The copy edits are mostly suggestions.

src/components/card/card.tsx Outdated Show resolved Hide resolved
src-docs/src/views/card/card_example.js Outdated Show resolved Hide resolved
src-docs/src/views/card/card_example.js Outdated Show resolved Hide resolved
src-docs/src/views/card/card_example.js Outdated Show resolved Hide resolved
src-docs/src/views/card/card_layout.js Outdated Show resolved Hide resolved
src-docs/src/views/card/card_layout.js Outdated Show resolved Hide resolved
src/components/card/_card.scss Show resolved Hide resolved
src/components/card/card.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Looks good!

src-docs/src/views/card/card_example.js Outdated Show resolved Hide resolved
Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>
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.

Thanks for the changes!

@cchaos cchaos merged commit 8e46668 into elastic:master Nov 27, 2019
@cchaos cchaos deleted the card-title-size branch November 27, 2019 14:02
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