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

How to determine which element ListGroup should output. #1181

Closed
apkiernan opened this issue Aug 18, 2015 · 8 comments
Closed

How to determine which element ListGroup should output. #1181

apkiernan opened this issue Aug 18, 2015 · 8 comments

Comments

@apkiernan
Copy link
Contributor

As mentioned in #1169 continuing discussion here.

Based on the discussion, it sounds like the current thinking is:

  • Check the children of ListGroup, and if they are ListGroupItems then we can render as we do currently
  • If they are not (user created custom components), then we would fall back to a componentClass prop that the user would specify whether to render <ul> or <div> and render the user specified element.
@taion
Copy link
Member

taion commented Aug 18, 2015

I think that's right. For the latter, div should probably be the default. I think this is the safest way to handle it.

@apkiernan
Copy link
Contributor Author

For the componentClass prop, I believe everywhere we use this prop its propType is elementType. Should we still use that here? I could see using it for consistency, but <ListGroup> should only be <div> or <ul> so I wonder if React.PropTypes.oneOf(['div', 'ul']) would be more accurate.

@AlexKVal
Copy link
Member

I vote for React.PropTypes.oneOf(['div', 'ul']) ❇️

@taion
Copy link
Member

taion commented Aug 26, 2015

Does it make sense to have a custom container component here?

@taion
Copy link
Member

taion commented Aug 26, 2015

I mean a custom React component.

@apkiernan
Copy link
Contributor Author

I can see using a custom component, but I think making it oneOf(['ul', 'div']) is the right call, as I think we want to ensure that the correct markup is being used.

@taion
Copy link
Member

taion commented Aug 31, 2015

I'm okay with that. It wouldn't be a breaking change to allow custom components down the road anyway.

@apkiernan apkiernan self-assigned this Sep 2, 2015
taion added a commit that referenced this issue Oct 1, 2015
[added] #1181 ListGroup supports componentClass prop
@taion
Copy link
Member

taion commented Oct 1, 2015

Fixed by #1348.

@taion taion closed this as completed Oct 1, 2015
egauci pushed a commit to egauci/react-bootstrap that referenced this issue Oct 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants