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

fix(number prop on dots): number prop on dots works when thumbnails a… #124

Closed
wants to merge 2 commits into from

Conversation

udaypydi
Copy link
Contributor

…re supplied. If the number isnt supplied then the length of thumbnails are considered

  • package.json:version has been updated with npm version patch (or major/minor as appropriate)
  • @brainhubeu/react-carousel version has been updated in docs-www/package.json to the same which is in package.json:version

…re supplied. If the number isnt supplied then the length of thumbnails are considered
@@ -27,20 +27,26 @@ export default class CarouselDots extends Component {

renderCarouselDots() {
if (this.props.thumbnails) {
return this.props.thumbnails.map((thumbnail, index) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be done simpler:

const dotsLength = isNaN(this.props.number) ? this.props.thumbnails.length : this.props.number;
return this.props.thumbnails.slice(0, dotsLength).map((thumbnail, index) => (

@piotr-s-brainhub
Copy link
Contributor

Thanks @udaypydi

@Lukasz-pluszczewski what do you think about this PR?

@piotr-s-brainhub
Copy link
Contributor

@udaypydi

you have conflicts now

@Lukasz-pluszczewski
Copy link
Member

Lukasz-pluszczewski commented Oct 2, 2019

Am I correct that these changes were already merged as apart of #126 ?

@piotr-s-brainhub
Copy link
Contributor

@udaypydi

could you reply?

@udaypydi
Copy link
Contributor Author

Yeah these changes are already merged in other PR.

@piotr-s-brainhub
Copy link
Contributor

thanks @udaypydi

so I'm closing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants