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 List Group component #1377

Merged
merged 20 commits into from
Jan 3, 2019
Merged

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Dec 14, 2018

🔨 This will be converted to TS in a follow-up PR 🔧


Summary

As a precursor to the K7 side nav redesign, this component provides a way to neatly display a list of items. In particular, it can handle link list items with an optional icon and an optional secondary action.

It starts with a basic list of items in plain text. The most likely use case for EUI users will be easily displaying a list of links. This can be accomplished by supplying a label and an href with an optional iconType. In the event users want advanced text formatting or button capabilities (such as onClick), they can pass other EUI component like EuiText and EuiButtonEmpty, respectively.

The more advanced use case will be for the K7 navigation. In this case, we'll need to supply not only an EuiButtonEmpty but also a secondary button action via linkAction.

For those reasons, both label and linkAction are set to PropTypes.node. I've provided an example of the more advanced K7 side nav use case, but perhaps we don't want to advertise that.

Screenshots

screenshot 2018-12-14 16 19 29

screenshot 2018-12-14 16 19 58

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

@ryankeairns
Copy link
Contributor Author

Note for link with action: I have a stash where the secondary button is positioned absolutely and the main button has the hover state (as opposed to the containing span).

I'll work that through a bit further and see if it simplifies things. I suspect it will make for better hover and focus states along with cleaner CSS. Related, you'll notice a slightly odd focus state when you navigate to and trigger the primary button action (the focus state does not fill the entire list item area).

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.

The design of these look great. I like that there are bordered and flushed options so the consumer doesn't have to think about the extra padding effects. My main concerns are around relying on importing other components and/or forcing the consumer to pass in certain components (like buttons if they want onClick).

This component, similar to the way we handle normal buttons, facet buttons, and badges can completely take care of the handling of onClick vs href vs extraAction. Meaning, it can do the logic of deciding what type of html element (span vs button vs anchor) to display and their states.

I've tried to describe how to change things in the code review, but lmk if you want to zoom over it.

src/components/list_group/list_group_item.js Outdated Show resolved Hide resolved
src/components/list_group/list_group_item.js Outdated Show resolved Hide resolved
src/components/list_group/list_group_item.js Outdated Show resolved Hide resolved
src/components/list_group/list_group_item.js Outdated Show resolved Hide resolved
src/components/list_group/list_group_item.js Outdated Show resolved Hide resolved
src/components/list_group/list_group.js Outdated Show resolved Hide resolved
src/components/list_group/_list_group.scss Outdated Show resolved Hide resolved
src/components/list_group/_list_group.scss Outdated Show resolved Hide resolved
src/components/list_group/_list_group.scss Outdated Show resolved Hide resolved
src/components/list_group/_list_group.scss Outdated Show resolved Hide resolved
@ryankeairns
Copy link
Contributor Author

For those reviewing, I'm currently working to make this component accept an array of list items, similar to the Description List component.

@ryankeairns ryankeairns changed the title [WIP] Add List Group component Add List Group component Dec 18, 2018
@ryankeairns
Copy link
Contributor Author

@cchaos , et al, this EuiListGroup PR is ready for a fresh review.

Thanks for the great feedback the first time around, this feels much more complete and organized!

Some notable changes:

  • link actions can now be used on any list item type (text, link, button, etc.)
  • the linkAction prop is now an object that spreads over an EuiButtonIcon
  • the logic for assembling the content has been tidied up
  • the pin/favorite mechanism in the docs now 'works' (its really basic functionality to demonstrate the use case)
  • split out the EuiListGroupItem CSS into its own file
  • EuiListGroup now accepts an array of items
  • EuiListGroupItem now uses straight HTML elements where it used to use EuiButtonEmpty
  • size prop was added for simple text styling; before you had to pass an EuiText component
  • tested in IE

@cchaos
Copy link
Contributor

cchaos commented Dec 21, 2018

@ryankeairns Here is a PR: ryankeairns#1

I tried to explain a bit of the new render schema and some caveats.

Ryan Keairns and others added 2 commits December 21, 2018 16:42
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.

Just some docs updates needed and it looks the :focus state with extraAction seems odd to me that it doesn't highlight the whole item.

src-docs/src/views/list_group/list_group_example.js Outdated Show resolved Hide resolved
src-docs/src/views/list_group/list_group_example.js Outdated Show resolved Hide resolved
src-docs/src/views/list_group/list_group_example.js Outdated Show resolved Hide resolved
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.

LGTM. We will eventually need to flesh out more of the jest tests too, but let's get this in to keep the ball rolling. 😃

@ryankeairns
Copy link
Contributor Author

@chandlerprall we're moving quick on this (I have another PR based on this one) and we'd like to get it in PMs hands. I'm going to merge this but please feel free to do a post-merge review!

@ryankeairns ryankeairns merged commit 0fe748a into elastic:master Jan 3, 2019
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