-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adding className to carousel components #682
Adding className to carousel components #682
Conversation
Addresses Issue 69 - Adds support for carousel components to accept the className prompt - Adds an example showing use of custom carousel tag and className
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some minor things
src/Carousel.js
Outdated
@@ -190,13 +191,15 @@ Carousel.propTypes = { | |||
// controls whether the slide animation on the Carousel works or not | |||
slide: PropTypes.bool, | |||
cssModule: PropTypes.object, | |||
className: PropTypes.string, | |||
}; | |||
|
|||
Carousel.defaultProps = { | |||
interval: 5000, | |||
pause: 'hover', | |||
keyboard: true, | |||
slide: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think you meant to put this here
src/CarouselCaption.js
Outdated
'carousel-caption', | ||
'd-none', | ||
'd-md-block' | ||
'd-md-block', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk if comma dangle in a function call is a thing
.custom-tag { | ||
max-width: 100%px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @TheSharpieOne
@@ -76,14 +76,13 @@ class Example extends Component { | |||
|
|||
return ( | |||
<div> | |||
<style dangerouslySetInnerHTML={{__html: ` | |||
<style>{` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better than what I had... I have a few other places to change now.
Please excuse the uninformed question. In particular, resizing it so that the image is responsive as well? |
Addresses Issue #669