feat(list-group): adds the pf list-group pattern

[priley86]

What:
This adds the list-group pattern to be used subsequently in Wizard #69 and other components.

Also updates package-lock.json / React Bootstrap / Prettier and renames the existing ListGroupItem for consistency.

Storybook:
https://rawgit.com/priley86/patternfly-react/list-group-storybook/index.html?knob-Label=Danger%20Will%20Robinson%21&selectedKind=ListGroup&selectedStory=Basic%20Example&full=0&down=1&left=1&panelRight=0&downPanel=storybooks%2Fstorybook-addon-knobs

Closes #73

[priley86]

Dev notes: i noted that in order for React Bootstrap to render the Linked items example correctly, I had to provide href="#" as href="" will not render the links/ a tag correctly.

[sharvit]

I think Badge should be a component:
https://react-bootstrap.github.io/components.html#badges

[priley86]

+1... i'll update this PR shortly

[priley86]

@sharvit i've added Badges component and made the bsStyle change for the contextual classes. Also re-pushed the storybook linked above...

[priley86]

it's green now... any more thoughts on this @mturley @sharvit @jgiardino ?

[sharvit]

Looks perfect for me accept a small naming wondering...

[sharvit]

Can't it be simpler as ListViewItem, ListViewItemHeader, ListViewItemContainer?

Maybe it will make life easier to also export them as ListView.Item, ListView.ItemHeader, ListView.ItemContainer to keep them contextual.

[priley86]

@sharvit actually, this name conflicts w/ the existing ListViewItem (so I don't think we should change ListViewGroupItem to ListViewItem).

If you see the ListView stories, you will see this...

Will these names work? I.e. ListView, ListView.Item, ListView.InfoItem, ListView.GroupItem, ListView.GroupItemHeader, ListView.GroupItemContainer,ListView.Expand, ListView.Checkbox, ListView.Actions, ListView.MainInfo, ListView.Left...

[sharvit]

Yes, I just look deeper into the ListView code and it makes sense to export all of them as contextual components.
It will make the usage much simpler because you will need to import only the ListView to use them all.

I think it will be even better to split the file ListView/index.js to a single file per component.
Then the ListView/index.js can be something like:

export { default as ListView } from './ListView';
export { default as ListViewItem } from './ListViewItem';
export { default as ListViewInfoItem } from './ListViewInfoItem';

ListView.Item = ListViewItem;
ListView.InfoItem = ListViewInfoItem;

[sharvit]

And I think that the Icon component can be based on a more basic Icon that can be reused.
We can use https://github.com/danawoodman/react-fontawesome and just export it as reusable Icon component.

[priley86]

Sounds great! I'll make these updates soon.

[priley86]

@sharvit @mturley i've split apart ListView and added the contextual names, i.e. ListView.Item etc etc.. After looking at this again, it seems best to use default exports and avoid having to a create a separate index.js inside each component (i.e. this would mean creating two two-line files everywhere), but feel free to send me a patch if you disagree...

As far as extending the React Bootstrap Es6 modules everywhere, I think this is good as it gives us the flexibility to make enhancements later if needed (as we did w/ the Button).

I've gone ahead and broken apart Dropdown (I noted it was referenced in DropdownKebab) and added our Icon component using react-fontawesome. I think using a name prop to signify a FontAwesome icon vs className only for a patternfly icon is flexible enough. thoughts?

p.s. @jgiardino - this adds an aria-hidden attribute for icons. Are we good w/ this or should that be optional too?

[sharvit]

I am suggesting to add a type prop that can be fa or pf
It will make the API around the Icon easier to understand

// In different file
const PatternflyIcon = ({ name, className, props }) => { 
  return (<span
    aria-hidden="true"
    className=cx('pficon', `pficon-${name}`, className)
    {...props}
  />);
}

PatternflyIcon.propTypes = {
  name: PropTypes.string.isRequired
}
  
const Icon = ({ type, ...props }) => {
  if (type === 'fa') {
    return <FontAwesome {...props} />
  }

  if (type === 'pf') {
    return <PatternflyIcon {...props} />
  }

  throw new Error(`Unsupported prop type=${type}`)
}

Icon.propTypes = {
  type: PropTypes.oneOf(['fa', 'pf']),
  name: PropTypes.string.isRequired
}

ListViewIcon.defaultProps = {
  type: 'fa'
}

[priley86]

I like it... I've updated it just now.

[sharvit]

Those imports can be simpler as:

import ListViewLeft from './ListViewLeft'
import ListViewMainInfo from './ListViewMainInfo'
import ListViewRow from './ListViewRow'

[priley86]

duh!

[priley86]

@sharvit updated - what else?

[sharvit]

Looks great 👍

[priley86]

ok - @mturley @jgiardino what else? I'd love to merge this and start rebasing #76 and #69

[mturley]

It looks like this snapshot test went from being shown to hidden? Do we not have a snapshot test for the dropdown shown case?

[priley86]

+1 I noticed the same. The react-fontawesome library used is small enough that I think it would be simple enough to incorporate our own implementation without it (i.e.: it's one component here: https://github.com/danawoodman/react-fontawesome/blob/master/src/index.js). Depending on @jgiardino's feedback, I may self implement it (as it looks like currently the aria-hidden is hard set to true.

[mturley]

This is fine as-is, but to avoid the repetitive return statement you could do:

const IconComponent = (type === 'fa' && FontAwesome || type === 'pf' && PatternflyIcon);
if (IconComponent) {
  return <IconComponent {...props} />
}
throw new Error(...)

[priley86]

I like it. Less code and accomplishes the same for now. I'll wait until @jgiardino responds and update the PR.

[mturley]

My only comments are very minor nitpicks, mostly this looks great. I'd be fine with merging it as-is or with these changes. Mostly I want to make sure we're not losing a test case.

[priley86]

@sharvit after some discussion w/ @jgiardino yesterday, i've opted to self include react-fontawesome so we can have more fine-grained control of the a11y settings in the future (it's only a single class).

I've made @mturley updates as well.

@sharvit ok to merge?

[jeff-phillips-18]

Patternfly is moving away from the list-view-pf implementation and suggesting the use of the list-pf implementation. Have you considered this?

[priley86]

Yes! I have started #63 to introduce the new list pattern. My understanding is that this existing pattern is in use in some other areas today still (and that we can likely remove this list view in the future). This PR kind of expanded when we discovered that ListGroup names in React-Bootstrap conflicted with the existing class names used. I think this PR can more generally be purposed to introduce List-Group, Icon, and the other basic components we can extend from React-Boostrap. Ideally we can start using these components throughout. Would also like to merge #77 soon so we can begin using Grid, Row, Col, etc.

thoughts on this @jeff-phillips-18 ?

[jeff-phillips-18]

OK, makes sense. Thanks.

[mturley]

Do we have the fact that we are moving away from the list-view-pf implementation documented somewhere? Should we add comments to this code to tell people they should consider using the list-pf components instead?

[priley86]

+1. I'm all for this. Should we wait until #63 is merged? My guess is we will have to support both for a bit. I am awaiting API feedback on #63 before adding the other list examples, but am happy to hand that over if someone else gets to it first. I'd just love getting the basics in this PR and #77 out of the way first so we can reuse many of these components throughout. My guess is forms and form components will be useful soon too. I think we are having some repo organization meetings next week that should help with this!

[mturley]

Sounds good. It will be good to document this somewhere though, I am probably going to use list-pf instead of list-view-pf for stuff in my vertical nav, only because I saw this PR.

[sharvit]

It seems that react-fontawesome has a good a11y implementation.
Maybe I am missing something but why do you want to get rid of it?

[mturley]

Are we ready to merge this today or tomorrow? I started basing my vertical-nav branch on it because I will need those <ListGroup>s.

[priley86]

I believe there was just a misunderstanding about the a11y yesterday. I've gone ahead and reverted this back to using react-fontawesome.

I'll merge this now as many other PRs are waiting to rebase at this point. I've opened #85 to capture any remaining issues we want to follow up on.