[added] Breadcrumb Component #1273
Conversation
| /** | ||
| * HTML id for the wrapper `ol` element | ||
| */ | ||
| id: React.PropTypes.string |
There was a problem hiding this comment.
I fixed it by passing {...props} into the ol element.
There was a problem hiding this comment.
What's the point in specifying this as a propType though? You're not using it in the component code, nor attaching any special semantics to it - it's just getting passed through as an HTML attribute. Better to just drop this propType entirely.
There was a problem hiding this comment.
I notice that the Nav.js also defines className and id propTypes, and pass {...this.props} to child component. https://github.com/react-bootstrap/react-bootstrap/blob/master/src/Nav.js#L20-L30. Is there any reason why Nav.js define them in propTypes? Thanks.
There was a problem hiding this comment.
In that case it's to clarify that id and className are separate from what's attached to the wrapped ul element for documentation. You don't have that here, so it's not relevant.
There was a problem hiding this comment.
Thanks for your explanation. I will remove the propTypes from the Breadcrumb.js.
|
Thanks for the PR and the extensive tests! Just left a few comments. |
|
Very nice. LGTM. Can you squash these down to a single commit? Need one more reviewer. |
|
Done! Thanks for your review 👍 |
|
Now it looks like: Then I would not use And this PR has to be rebased against the latest All other LGTM P.S. If @ycavatars you want, (and nobody else mind it), I could myself fix all those small "issues" just right after merging. 🍒 |
|
@AlexKVal Yes, please help me fix the issues 👍 |
I added
Breadcrumbcomponent for issue #56. Please review this request, thanks :)