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

Selectable EuiCards #1895

Merged
merged 12 commits into from
May 1, 2019
Merged

Selectable EuiCards #1895

merged 12 commits into from
May 1, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Apr 29, 2019

Fixes #1868

This PR adds a selectable: {} prop that requires at the very least an onClick to be able to adde a "Select" button to the bottom of an EuiCard. This allows for a selectable pattern to be achieved without worrying about links that may occur inside descriptions or footers as the select button is distinct and not the entire card.

The new EuiCardSelect component is not exported via the index file as it is a component only used by EuiCard and therefore should not be found outside of EUI.

Screen Shot 2019-04-29 at 14 21 50 PM

Reviewers note:

The above example and currently in the docs, I'm showing all the permutations for easier testing, however, I'll reduce the doc example to only the first row as the recommended and default usage.

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
  • [ ] This required updates to Framer X components

@snide
Copy link
Contributor

snide commented Apr 29, 2019

cc @gjones so he's aware he'll have some new tooling for cards.

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.

This looks nice, I'm sure it will get a lot of use!

A few observations, the first of which may be the only potential 'blocker':

  • When clicking/toggling the selected state, the custom (not check mark) icon does not go away when the card is deselected. Given this is a key visual differentiator between states, I suspect it should not display in the unselected state (see the example with the bell icon).
    icon-hide

  • I'm reminded that the bottomGraphic needs to be removed since we are no longer using that design on the new home page. I have a to-do to tackle this.

  • And last but not least, the most subjective piece of feedback :) Having played with the styles a little bit, a colored border (while perhaps not quite as visually appealing to some) does a better job of emphasizing the selected state. I suspect this would be most beneficial when a user is picking from multiple rows of options. Related, it seems a selected state here would translate to the card (button) being pressed. Coupling these things, the selected state could use a colored border and no shadow to provide more visual emphasis.

Screenshot 2019-04-29 15 50 07

@cchaos
Copy link
Contributor Author

cchaos commented Apr 29, 2019

Thx @ryankeairns

When clicking/toggling the selected state, the custom (not check mark) icon does not go away when the card is deselected.

Yes, this is on purpose. This is to show that the overrides will override all states. They can insert logic if they want the checkmark back on selected states like:

iconType: this.state.cardIsSelected ? 'check' : 'bell',

Since they have to manage the selected state anyway, this is the most explicit way for them to know when which icon will show. Otherwise it would be a mess of having to provide three full objects for all three states.

I've updated the danger example to highlight this:


I also pushed an example of altering the border colors. I'm on the fence with it. I think it might be too much, but maybe that's a good thing?

@cchaos cchaos requested a review from ryankeairns April 30, 2019 14:21
@ryankeairns
Copy link
Contributor

ryankeairns commented Apr 30, 2019

My take is that the border color is valuable and makes a worthwhile impact on clarity. Given that users can use their own styling on the bottom button area, I think it's even more necessary. As in the example, the background color and icon may not switch between the selected/unselected state, so the border insures that a noticeable amount of visual differentiation will remain. Thanks for trying this out!

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.

👍 LGTM on the visual side. Scanned through the code, nothing jumped out at me there.

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.

Code looks good. Pulled and tested locally, also

@cchaos cchaos merged commit 7753618 into elastic:master May 1, 2019
@cchaos cchaos deleted the selectable-cards branch May 1, 2019 16:49
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.

Selectable pattern for EuiCard
4 participants