Skip to content

Commit

Permalink
PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
William Chou committed Jan 3, 2017
1 parent 6cb6572 commit 158f14a
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 43 deletions.
2 changes: 1 addition & 1 deletion examples/bind.amp.html
View file
Expand Up @@ -98,7 +98,7 @@ <h2>Linked carousels</h2>
</amp-carousel>

<p>With &lt;amp-selector&gt;</p>
<amp-selector layout=container name="carousel-selector" [selected]="selectedSlide" on="select:AMP.setState(selectedSlide=event.option)">
<amp-selector layout=container name="carousel-selector" [selected]="selectedSlide" on="select:AMP.setState(selectedSlide=event.targetOption)">
<amp-carousel controls width=400 height=100>
<amp-img src="https://ampbyexample.com/img/image1.jpg" width=100 height=75 option=0></amp-img>
<amp-img src="https://ampbyexample.com/img/image2.jpg" width=100 height=75 option=1></amp-img>
Expand Down
24 changes: 16 additions & 8 deletions extensions/amp-carousel/0.1/slidescroll.js
View file
Expand Up @@ -278,7 +278,7 @@ export class AmpSlideScroll extends BaseSlides {
(dir == 1 && !hasPrev) ? 0 : this.slideWidth_;
this.customSnap_(currentScrollLeft, dir);
} else {
this.showSlide_(newIndex, /* opt_isUserAction */ true);
this.showSlideAndTriggerAction_(newIndex);
}
}
}
Expand Down Expand Up @@ -436,7 +436,7 @@ export class AmpSlideScroll extends BaseSlides {
this.slidesContainer_.classList.add('-amp-no-scroll');
}
// Scroll to new slide and update scrollLeft to the correct slide.
this.showSlide_(newIndex, /* opt_isUserAction */ true);
this.showSlideAndTriggerAction_(newIndex);
this.vsync_.mutate(() => {
if (this.isIos_) {
// Make the container scrollable again to enable user swiping.
Expand All @@ -451,10 +451,9 @@ export class AmpSlideScroll extends BaseSlides {
* Makes the slide corresponding to the given index and the slides surrounding
* it available for display.
* @param {number} newIndex Index of the slide to be displayed.
* @param {boolean=} opt_isUserAction
* @private
*/
showSlide_(newIndex, opt_isUserAction) {
showSlide_(newIndex) {
const noOfSlides_ = this.noOfSlides_;
if (newIndex < 0 ||
newIndex >= noOfSlides_ ||
Expand Down Expand Up @@ -499,11 +498,20 @@ export class AmpSlideScroll extends BaseSlides {
this.slideIndex_ = newIndex;
this.hideRestOfTheSlides_(showIndexArr);
this.setControlsState();
}

if (opt_isUserAction) {
const event = new CustomEvent('SlideChange', {detail: {slide: newIndex}});
this.action_.trigger(this.element, 'slidechange', event);
}
/**
* Shows the slide at the given index and triggers a `slidechange` action.
* @param {number} newIndex
* @private
*/
showSlideAndTriggerAction_(newIndex) {
this.showSlide_(newIndex);

const name = 'slidechange';
const detail = {slide: newIndex};
const event = new CustomEvent(`slidescroll.${name}`, {detail});
this.action_.trigger(this.element, name, event);
}

/**
Expand Down
84 changes: 50 additions & 34 deletions extensions/amp-selector/0.1/amp-selector.js
View file
Expand Up @@ -75,27 +75,34 @@ export class AmpSelector extends AMP.BaseElement {
mutatedAttributesCallback(mutations) {
mutations.forEach(mutation => {
const newValue = mutation.newValue;

switch (mutation.name) {
case 'selected':
this.clearAllSelections_();
if (newValue) {
// Create a query with an attribute selector for every
// comma-delimited option value in `newValue`.
const options = String(mutation.newValue).split(',');
const selectors = [];
for (let i = 0; i < options.length; i++) {
// Only use first value if multiple selection is disabled.
if (i > 0 && !this.isMultiple_) {
break;
}
selectors.push(`[option='${options[i]}']`);
if (!newValue) {
this.clearAllSelections_();
return;
}
// Generate map from comma-delimited option values in `newValue`.
const selectedArray = String(newValue).split(',');
const selectedMap = selectedArray.reduce((map, value) => {
map[value] = true;
return map;
}, {});
// Iterate through elements and toggle selection as necessary.
for (let i = 0; i < this.options_.length; i++) {
// Only use first value if multiple selection is disabled.
if (i > 0 && !this.isMultiple_) {
break;
}
const query = selectors.join(',');
const elements = this.element.querySelectorAll(query);
for (let i = 0; i < elements.length; i++) {
this.setSelection_(element[i]);
const element = this.options_[i];
const option = element.getAttribute('option');
if (selectedMap[option]) {
this.setSelection_(element);
} else {
this.clearSelection_(element);
}
}
// Update inputs.
this.setInputs_();
break;
}
Expand Down Expand Up @@ -124,13 +131,17 @@ export class AmpSelector extends AMP.BaseElement {
}

/**
* Creates inputs for the currently selected elements.
* Creates inputs for the currently selected elements and returns a string
* array of their option values.
* @note Ignores elements that have `disabled` attribute set.
* @return {!Array<string>}
* @private
*/
setInputs_() {
const selectedValues = [];
const elementName = this.element.getAttribute('name');
if (!elementName || this.isDisabled_) {
return;
return selectedValues;
}
const formId = this.element.getAttribute('form');

Expand All @@ -143,17 +154,20 @@ export class AmpSelector extends AMP.BaseElement {
this.selectedOptions_.forEach(option => {
if (!option.hasAttribute('disabled')) {
const hidden = doc.createElement('input');
const value = option.getAttribute('option');
hidden.setAttribute('type', 'hidden');
hidden.setAttribute('name', elementName);
hidden.setAttribute('value', option.getAttribute('option'));
hidden.setAttribute('value', value);
if (formId) {
hidden.setAttribute('form', formId);
}
this.inputs_.push(hidden);
fragment.appendChild(hidden);
selectedValues.push(value);
}
});
this.element.appendChild(fragment);
return selectedValues;
}

/**
Expand All @@ -171,29 +185,31 @@ export class AmpSelector extends AMP.BaseElement {
if (!el || el.hasAttribute('disabled')) {
return;
}
/** @type {?Array<string>} */
let selectedValues;
if (el.hasAttribute('selected')) {
if (this.isMultiple_) {
this.clearSelection_(el);
this.setInputs_();
selectedValues = this.setInputs_();
}
} else {
this.setSelection_(el);
this.setInputs_();
selectedValues = this.setInputs_();
}

// Trigger 'select' event with two data params:
// 'option' - option value of the selected or deselected element.
// 'options' - comma-delimited option values of all selected elements.
const options = [];
this.selectedOptions_.forEach(element => {
options.push(element.getAttribute('option'));
});
const detail = {
option: el.getAttribute('option'),
options: options.join(','),
};
const selectEvent = new CustomEvent('Select', {detail});
this.action_.trigger(this.element, 'select', selectEvent);
// Don't trigger action if selected values haven't changed.
if (selectedValues) {
// Trigger 'select' event with two data params:
// 'targetOption' - option value of the selected or deselected element.
// 'selectedOptions' - comma-delimited option values of selected elements.
const name = 'select';
const detail = {
targetOption: el.getAttribute('option'),
selectedOptions: selectedValues.join(','),
};
const selectEvent = new CustomEvent(`amp-selector.${name}`, {detail});
this.action_.trigger(this.element, name, selectEvent);
}
}

/**
Expand Down

0 comments on commit 158f14a

Please sign in to comment.