Added prevIconClass and nextIconClass props to Carousel with defaults…

[mcraiganthony]

Allow specifying alternative icon classes in Carousel (refer to issue #502). I added two new props to the Carousel component that allow alternative icon classes to be passed to the previous and next buttons. If alternative icon classes are not passed, the Carousel defaults to using glyphicons. This gives developers the ability to use their own set of icons instead of being forced to use glyphicons.

Added two new props to the Carousel component:
prevIconClass: React.PropTypes.string
nextIconClass: React.PropTypes.string

Added default props for the two new props above:
prevIconClass: 'glyphicon glyphicon-chevron-left',
nextIconClass: 'glyphicon glyphicon-chevron-right'

[taion]

Huh. Why don't:

react-bootstrap/src/Carousel.js

Line 161 in d9b08d3

<span className="glyphicon glyphicon-chevron-left" />

react-bootstrap/src/Carousel.js

Line 169 in d9b08d3

<span className="glyphicon glyphicon-chevron-right"/>

use the Glyphicon component?

I'd actually implement this as a replacement for the element inside the anchor, and default to the respective Glyphicons.

[mfunkie]

I would say as long as the glyphicons don't use a component, that class should be swappable. Maybe otherwise there could be a renderPrev and renderNext prop that defaults to

renderPrev: () => <span className="glyphicon glyphicon-chevron-left" />
renderNext: () => <span className="glyphicon glyphicon-chevron-right" />

However, many people using Bootstrap don't use glyphicons and this pull request seems to follow an existing pattern in code.

[mcraiganthony]

@mfunkie presents the ideal solution. Being able to dump whatever you like inside of the anchor tag would give us the most flexibility.

Since the glyphicon component wasn't being used in the Carousel, I was simply trying to stick with the established pattern.

[taion]

I mean set the default value to: <Glyphicon icon="chevron-right" />, &c, and make the propType be React.PropTypes.node.isRequired.

The former is just a cleanup, and the latter seems to be a more correct way to model it than accepting a function or class name or whatever.

[mtscout6]

I agree that allowing for the Glyphicon component and possibly swapping for something else would be ideal. For example allowing use of: https://github.com/andreypopp/react-fa.

I don't know if the Carousel pre-dates the Glyphicon component or not, but definitely precedes my time with the library.

A few more issues:

• This change should also include tests that assert that the changed props are applied.
• Added documentation for this feature, I wouldn't bring in FontAwesome or anything. Simply showing use of either different icons from Glyphicon or a custom component would suffice.

[mfunkie]

@taion Would it be a required prop if we have a default value?

@mtscout6 The existing documentation for Carousel is a bit sparse, and thus overriding prev/next might seem out of place as far as I can see on the GitHub Pages site. Are there any plans to bring in usage of something like react-docgen and start documenting the propTypes in React Bootstrap?

[taion]

I'm not 100% sure on the isRequired part, but it seems like it makes sense, independent of the presence of a default value.

[mtscout6]

@mfunkie See #431 (I know you got that message from the slack room, just sharing for posterity)

[mtscout6]

@mfunkie It would still be required if that default prop was removed so yes I think it should still be required.

[mfunkie]

Maybe my understanding of isRequired is flawed. As far as I know, isRequired's use case is only if there are no default props, otherwise it's not technically a required prop. Otherwise, all props would be 'isRequired', even if the component fails gracefully.

[mtscout6]

I could be flawed as well, either should be fine I guess. Since there is a default than there will always be one, we can easily revisit the isRequired option if the default is ever removed. Which is partly why I don't think it hurts for it to be there.

[mcraiganthony]

Per @taion 's suggestion ...

Added two new props to the Carousel component:
prevIcon: React.PropTypes.node.isRequired
nextIcon: React.PropTypes.node.isRequired

Added default props for the two new props above:
prevIcon: ,
nextIcon:

[taion]

Looks good now. Do we need tests for this? I think

react-bootstrap/test/CarouselSpec.js

Lines 109 to 110 in 12d6ddb

	let backButtons = ReactTestUtils.scryRenderedDOMComponentsWithClass(instance, 'left');
	let nextButtons = ReactTestUtils.scryRenderedDOMComponentsWithClass(instance, 'right');

covers that the anchors get rendered appropriately.

Will need to squash before merge.

[mtscout6]

Tests would be appropriate to ensure those props are respected and not ignored.

[mcraiganthony]

@taion @mtscout6 Added unit test and squashed commits.

[mtscout6]

One last minor issue, can you amend your commit message to follow our guidelines as outlined in the Contributing Guide This will ensure that this change in documented in our changelog via our automated changelog tool.

[mathieumg]

As far as I know, isRequired's use case is only if there are no default props, otherwise it's not technically a required prop. Otherwise, all props would be 'isRequired', even if the component fails gracefully.

I was thinking the same, it allows you to glance at propTypes only (or documentation generated from it) and know what you have to provide when rendering that component. Otherwise, in what case would a prop not be required?

[taion]

LGTM

[mtscout6]

@mathieumg I wasn't thinking of auto generated docs, in which case you're right we shouldn't use isRequired. We aren't there yet with auto docs, but definitely good to consider.

@mcraiganthony Sorry to go back and forth on that. It's an issue that I myself have not investigated thoroughly enough, so I'll take the blame for how that went down. Can you go ahead and remove the isRequired flag? Otherwise this looks good to me.

[mtscout6]

I'll go ahead and remove those, sorry for all the confusion.

[mtscout6]

isRequired prop removed in 5e4ff2f

[AlexKVal]

👍

[mcraiganthony]

Thanks @mtscout6 . Much appreciated.