[fixed] ListGroup and ListGroupItem output correct elements.

[apkiernan]

Fix for #460 - ListGroup outputs <ul> or <div> depending on ListGroupItem (defaults to <ul> if no ListGroupItem). ListGroupItem outputs <li> by default, or <a> if 'href' prop is set.

[mtscout6]

Thanks for jumping in on this, I had originally thrown that issue out there as a reminder for myself to dig into that more.

I hadn't realized when I made that issue that Bootstrap supported both. I'm amazed at how much I learn something new everyday.

Gaining this new insight I think we should do a little more here. Realistically the two options should not be mixed and matched. Which I believe is partly the reason for the spans before. Consider the following example:

<ListGroup>
  <ListGroupItem>Non Link</ListGroupItem>
  <ListGroupItem href='#'>Link</ListGroupItem>
  <ListGroupItem onClick={function() { alert('clicked'); }}>Link</ListGroupItem>
</ListGroup>

In this case what do we do? The first item suggests uls with lis. Yet the second two suggest divs with as. Although the last should really be a button for accessibility and proper semantic html purposes. For clarity sake I tried the following raw html:

<div>
  <ul class="list-group">
    <li class="list-group-item">Dapibus ac facilisis in</li>
    <li class="list-group-item">Cras sit amet nibh libero</li>
    <li class="list-group-item">Porta ac consectetur ac</li>
    <li class="list-group-item">
      <a href='#'>Vestibulum at eros</a>
    </li>
  </ul>
  <div class="list-group">
    <a href="#" class="list-group-item">Dapibus ac facilisis in</a>
    <a href="#" class="list-group-item">Cras sit amet nibh libero</a>
    <a href="#" class="list-group-item">Porta ac consectetur ac</a>
    <span class="list-group-item">Porta ac consectetur ac</span>
    <button class="list-group-item">Vestibulum at eros</button>
  </div>
</div>

Which produces:

Issues to solve:

• Check all children for hrefs.
• Still allow for the span when rendering a parent div and the item is not a link.
• Figure out a solution without anchors for items with onClick (We may need to do this in a separate PR)
  • Option 1: Render a button and add documentation for css to include in your project to display correctly.
  • Option 2: Keep it as an anchor tag (:-1: I'm not a fan of this)
  • Option ?: Suggestions welcome

[apkiernan]

Should we wait for guidance on the bootstrap issue before going forward on this?

[mtscout6]

For right now, I think as long as the first two issues are solved then we can bring this in. The main issue I see with your current implementation is that you only check the first child to see if it has an href, when you should check to see if any of the children do. Then when you are rending the parent with a <div> then you'll still need to support <span> in case there is a list item that does not have an href or onClick handler. Button support in lieu of a link will need to wait for twbs/bootstrap#16215. Once twbs/bootstrap#16204 is resolved then we can follow that up with an action plan. I'll update the original issue to reflect the long term goals.

[apkiernan]

I believe the first two issues are all set now.

ListGroup renders:

• <div> when the children are spans/anchors,
• <ul> when children are list items (which occurs when no children are anchors), and
• <div> when there are no children.

ListGroupItem now renders the appropriate element based on ListGroup.

If there are any other issues, let me know.

[mtscout6]

Using Array.prototype.some here would be a little faster since it immediately returns on the first true result. Also slightly cleaner code.

[AlexKVal]

As for Array.isArray(this.props.children)

The this.props.children has opaque type and structure.
Consequently it can be subject to changes in future React releases.
Perhaps it would be more appropriate to use here React.Children API ?

[apkiernan]

Made the changes to use React.Children and Array.prototype.some, but this made me think if we use React.Children because props.children is opaque, should we use the API in place of Array.prototype.some as well? This would limit us to using React.Children.forEach since the API doesn't have a some method.

[mtscout6]

This looks great. I don't think we should worry too much about React.Children for now. If React changes things than we'll need to adjust but this will be faster for now.

[AlexKVal]

I don't think we should worry too much about React.Children for now. If React changes things than we'll need to adjust but this will be faster for now.

Totally agree. Thank you @apkiernan.