-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Add support for Bind's linked carousels use case #6797
Add support for Bind's linked carousels use case #6797
Conversation
@@ -479,6 +499,11 @@ export class AmpSlideScroll extends BaseSlides { | |||
this.slideIndex_ = newIndex; | |||
this.hideRestOfTheSlides_(showIndexArr); | |||
this.setControlsState(); | |||
|
|||
if (opt_isUserAction) { |
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.
i think we should make this var something like , !isBindAction
it would be hard to quantify what is userAction right?
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.
The idea is I only want to trigger an action if showSlide_()
is invoked as a result of a user gesture (e.g. tap/swipe rather than during initialization). IMO isBindAction
is not a good name here since it's not actually caused by Bind.
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.
in that case lets have a wrapper and not tax showSlide_ with an extra param.
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.
Done.
options.push(element.getAttribute('option')); | ||
}); | ||
const detail = { | ||
option: el.getAttribute('option'), |
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.
lets call this targetOption
and the other one selectedOptions
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.
Done.
// 'options' - comma-delimited option values of all selected elements. | ||
const options = []; | ||
this.selectedOptions_.forEach(element => { | ||
options.push(element.getAttribute('option')); |
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.
probably better to maintain a value array in setInputs_
rather than reading from the DOM again
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.
Added a return value to setInputs_
. I think this is better than maintaining an instance var since it requires less state tracking and avoids confusion about when it's updated.
const selectors = []; | ||
for (let i = 0; i < options.length; i++) { | ||
// Only use first value if multiple selection is disabled. | ||
if (i > 0 && !this.isMultiple_) { |
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.
splice the array before the loop and we can get rid of breaks in the loop.
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.
Creating a new array is less performant and not as readable IMO.
selectors.push(`[option='${options[i]}']`); | ||
} | ||
const query = selectors.join(','); | ||
const elements = this.element.querySelectorAll(query); |
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.
on a second thought
lets loop through this.options_
and see if the value is in the newSelectionList
and setSelection
or clearSelection
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.
Done.
@@ -479,6 +499,11 @@ export class AmpSlideScroll extends BaseSlides { | |||
this.slideIndex_ = newIndex; | |||
this.hideRestOfTheSlides_(showIndexArr); | |||
this.setControlsState(); | |||
|
|||
if (opt_isUserAction) { |
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.
in that case lets have a wrapper and not tax showSlide_ with an extra param.
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.
Thanks for the review.
@@ -479,6 +499,11 @@ export class AmpSlideScroll extends BaseSlides { | |||
this.slideIndex_ = newIndex; | |||
this.hideRestOfTheSlides_(showIndexArr); | |||
this.setControlsState(); | |||
|
|||
if (opt_isUserAction) { |
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.
Done.
const selectors = []; | ||
for (let i = 0; i < options.length; i++) { | ||
// Only use first value if multiple selection is disabled. | ||
if (i > 0 && !this.isMultiple_) { |
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.
Creating a new array is less performant and not as readable IMO.
selectors.push(`[option='${options[i]}']`); | ||
} | ||
const query = selectors.join(','); | ||
const elements = this.element.querySelectorAll(query); |
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.
Done.
// 'options' - comma-delimited option values of all selected elements. | ||
const options = []; | ||
this.selectedOptions_.forEach(element => { | ||
options.push(element.getAttribute('option')); |
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.
Added a return value to setInputs_
. I think this is better than maintaining an instance var since it requires less state tracking and avoids confusion about when it's updated.
options.push(element.getAttribute('option')); | ||
}); | ||
const detail = { | ||
option: el.getAttribute('option'), |
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.
Done.
case 'slide': | ||
const slide = parseInt(newValue, 10); | ||
user().assert(isFinite(slide), 'Invalid [slide] value: %s', newValue); | ||
this.showSlide_(slide); |
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.
Why doesn't this deserve an event?
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.
mutatedAttributesCallback
is not a direct result of user action; it's called when a bound attribute has its value changed during a Bind digest.
When to fire events/actions is a design choice in Bind to avoid cycles. More discussion here.
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.
because this was invoked by another event and we would not want to have an infinite event cycle?
const name = 'select'; | ||
const detail = { | ||
targetOption: el.getAttribute('option'), | ||
selectedOptions: selectedValues.join(','), |
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.
what if a value contains a comma inside of it? may be we should come up with a universal separator (and have this validated)
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 point, though any separator we'd pick would pose a similar problem. Changed mutatedAttributesCallback
to allow Array values to avoid string conversion entirely.
const newValue = mutation.newValue; | ||
|
||
switch (mutation.name) { | ||
case 'selected': |
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.
could we not re-use the clickHandler_ (we could create a method handleChenge_(element)
) for this case - would the mutation object have the target element triggering on which the change happens?
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.
Moved to a new private method selectedAttributeMutated_
.
return; | ||
} | ||
// Generate map from comma-delimited option values in `newValue`. | ||
const selectedArray = String(newValue).split(','); |
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.
i think we will have to come up with something better than , to split.
const selectedMap = selectedArray.reduce((map, value) => { | ||
map[value] = true; | ||
return map; | ||
}, {}); |
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.
could we not just check for selectedOptions.indexOf(this.options_[i]) ? i dont think we need reduce
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.
also we could do selectedOptions = [selectedOptions[0]] , if you dont like splice for the multiple selection case and we could get rid of the break condition below.
IMO splice on such a small array is probably negligible and break is certainly confusing :)
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.
I used indexOf
originally but thought O(n * m)
performance was not great. Reverted to use indexOf
which should be fine as long as number of selections is small.
Changed to use slice
since you insist. :)
// Exit if `element` is already selected. | ||
if (this.selectedOptions_.indexOf(element) >= 0) { | ||
return; | ||
} |
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.
im ok with this change but we should not be calling this on already selected elements :)
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.
Acknowledged. :)
Note that this could happen in this PR since we iterate over this.options_
.
case 'slide': | ||
const slide = parseInt(newValue, 10); | ||
user().assert(isFinite(slide), 'Invalid [slide] value: %s', newValue); | ||
this.showSlide_(slide); |
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.
because this was invoked by another event and we would not want to have an infinite event cycle?
* @param {number} newIndex | ||
* @private | ||
*/ | ||
showSlideAndTriggerAction_(newIndex) { |
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.
see if we can trigger this action on goCallback
in base-slides.js and then we will be able to avoid this wrapper completely.
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.
I think we still need it since it's called from updateOnScroll_
and scroll events are not supported by the base class.
158f14a
to
3dc5c14
Compare
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.
LGTM - please add tests :)
3dc5c14
to
6a0101f
Compare
Good call, added tests. |
newValue: 2, | ||
} | ||
]); | ||
expect(showSlideSpy).to.have.been.calledWith(2); |
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.
we should also test for how we handle invalid values
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.
Done.
newValue: '3', | ||
}]); | ||
|
||
expect(impl.options_[0].hasAttribute('selected')).to.be.false; |
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.
also check of setInputs_ is being called after each mutate.
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.
Done.
6a0101f
to
0b75da1
Compare
0b27871
to
0c6b565
Compare
* initial commit for linked carousels support * fix lint and type errors * PR comments * more PR comments * add unit tests * PR comments and API updates * add tag to warn()
* initial commit for linked carousels support * fix lint and type errors * PR comments * more PR comments * add unit tests * PR comments and API updates * add tag to warn()
Partial for #6199.
Adds partial binding support to
<amp-carousel>
and<amp-selector>
for the "linked carousels" Bind use case.<amp-carousel>
and add eventslidechange
with paramslide
<amp-selector>
and add eventselect
with paramsoption
andoptions
examples/bind.amp.html
.Requires #6779 to be merged first.
/to @camelburrito /cc @ericlindley-g