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.

Sign up for GitHub

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

Jump to bottom

Add ImageCarousel #457

Merged
merged 11 commits into from
Sep 13, 2018
Merged

Add ImageCarousel #457

merged 11 commits into from
Sep 13, 2018

Conversation

gthomas-appfolio
Copy link
Contributor

WIP, do not merge

@wpliao1989
Copy link
Contributor

I removed the usage of internal state because I wasn't able to get it working nicely in listings app.
The reason is that when the isOpen prop changes, it doesn't trigger an internal state change. So if the initial isOpen prop is false, the internal state would be initialized to false. Any subsequent change to isOpen doesn't change the internal change thus the modal will never show up.

@wpliao1989
Copy link
Contributor

wpliao1989 commented Sep 12, 2018 •
edited
Loading

Also, for some reason, autoPlay doesn't work. It always auto scrolls.

@gthomas-appfolio
Copy link
Contributor Author

autoPlay seems like issue with the storybook knobs, if you hardcode it works fine.

@wpliao1989
wl - Fix an issue where autoPlay doesn't work
9ed3927 
autoPlay wasn't working because in reactstrap there's this line:
https://github.com/reactstrap/reactstrap/blob/6.4.0/src/Carousel.js#L35
If we look closely we'll notice on this line
https://github.com/reactstrap/reactstrap/blob/6.4.0/src/Carousel.js#L57
calls setInterval if props.interval isn't falsy, even if autoPlay is set to false!

The workaround here is set interval to zero to avoid this.
@gthomas-appfolio gthomas-appfolio merged commit 76b0cfc into master Sep 13, 2018
@gthomas-appfolio gthomas-appfolio deleted the addImageCarousel branch September 13, 2018 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wpliao1989 wpliao1989 wpliao1989 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gthomas-appfolio @wpliao1989