Skip to content

added controlledItemIndex so the central can be controlled by parent#3

Open
ganmor wants to merge 2 commits intoroeierez:masterfrom
ganmor:controlled-index
Open

added controlledItemIndex so the central can be controlled by parent#3
ganmor wants to merge 2 commits intoroeierez:masterfrom
ganmor:controlled-index

Conversation

@ganmor
Copy link

@ganmor ganmor commented Jan 4, 2016

Sorry for not getting back earlier about the previous pull request, and thanks for integrating the changes.
This pull request allow the carousel to be controlled by the parent component and thus the url.

Also do you have any thought on how to implement a scroll method that takes velocity / animation duration as parameters ? ( if the scroller is going to be controlled by the url it would be much more convenient to have those exposed )

@roeierez
Copy link
Owner

roeierez commented Jan 5, 2016

I like the use of componentWillReceiveProps.
I think we don't need two 'controlled' indexes, we can use the 'initialItemIndex' also as you suggested in 'componentWillReceiveProps' and it will let the user scroll to an item on mounting and also whenever he changes the props.
If you think like me and willing to do these changes I will merge it as is.
Let me know what you think, and thanks again.

@roeierez
Copy link
Owner

roeierez commented Jan 5, 2016

Regarding controlling the animation duration.
Currently there is a constant in the scroller called: SCROLLING_TIME_CONSTANT
as higher it is as longer is the animation. you can expose it as props in the carousel and propagate it to the scroller.

@ganmor
Copy link
Author

ganmor commented Jan 12, 2016

I will submit a PR.
What I am thinking about when talking about scrolling animation is something like this.

  componentWillReceiveProps: function(newProps, newState) {
            if (newProps.controlledItemIndex && newProps.controlledItemIndex !== this.props.controlledItemIndex) {
                this.refs.scroller.scrollWithAnimation( (this.getItemWidth() + this.getItemsSpacing()) * (newProps.controlledItemIndex || 0) );
            }
        },

notice the scrollWithAnimation.

Thanks for the answer anyway, I will try to submit a different PR with a possible solution for the scrollWithAnimation.

@ganmor
Copy link
Author

ganmor commented Jan 14, 2016

PR updated

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.

2 participants