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

<amp-selector> element toggle the 'selected' attribute #13553

Merged
merged 17 commits into from
Feb 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions extensions/amp-selector/0.1/amp-selector.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,16 @@ export class AmpSelector extends AMP.BaseElement {
const delta = (args && args['delta'] !== undefined) ? args['delta'] : 1;
this.select_(delta);
}, ActionTrust.LOW);

this.registerAction('toggle', invocation => {
const args = invocation.args;
user().assert(args['index'] >= 0, '\'index\' must be greater than 0');
user().assert(args['index'] < this.options_.length, '\'index\' must be ' +
'less than the length of options in the <amp-selector>');
if (args && args['index'] !== undefined) {
this.toggle_(args['index'], args['value']);
}
}, ActionTrust.LOW);
}

/** @override */
Expand Down Expand Up @@ -331,6 +341,34 @@ export class AmpSelector extends AMP.BaseElement {
}
}

/**
* Handles toggle action.
* @param {number} index
* @param {boolean=} opt_value
*/
toggle_(index, opt_value) {
// Change the selection to the next element in the specified direction.
// The selection should loop around if the user attempts to go one
// past the beginning or end.
const indexCurrentStatus = this.options_[index].hasAttribute('selected');
const indexFinalStatus =
Copy link
Contributor

Choose a reason for hiding this comment

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

The !! shouldn't be necessary here, since value and indexCurrentStatus are already boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

opt_value !== undefined ? opt_value : !indexCurrentStatus;
const selectedIndex = this.options_.indexOf(this.selectedOptions_[0]);

if (indexFinalStatus === indexCurrentStatus) {
return;
}

// There is a change of the `selected` attribute for the element
if (selectedIndex !== index) {
this.setSelection_(this.options_[index]);
this.clearSelection_(this.options_[selectedIndex]);
} else {
this.clearSelection_(this.options_[index]);
}
}


/**
* Handles selectUp events.
* @param {number} delta
Expand Down
79 changes: 78 additions & 1 deletion extensions/amp-selector/0.1/test/test-amp-selector.js
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,84 @@ describes.realWin('amp-selector', {
expect(setInputsSpy).calledTwice;
});

it('should trigger `toggle` action even when no `value` argument is' +
' provided to the function', () => {
const ampSelector = getSelector({
config: {
count: 5,
selectedCount: 1,
},
});
ampSelector.build();
const impl = ampSelector.implementation_;

expect(ampSelector.hasAttribute('multiple')).to.be.false;
expect(ampSelector.children[0].hasAttribute('selected')).to.be.true;

// Test the case where the element to be `selected` and the currently
// selected element are different
let args = {'index': 2};
impl.executeAction(
{method: 'toggle', args, satisfiesTrust: () => true});
expect(ampSelector.children[0].hasAttribute('selected')).to.be.false;
expect(ampSelector.children[2].hasAttribute('selected')).to.be.true;

// Test the case where the element to be `selected` and the currently
// selected element are the same
args = {'index': 2};
impl.executeAction(
{method: 'toggle', args, satisfiesTrust: () => true});
expect(ampSelector.children[2].hasAttribute('selected')).to.be.false;
});

it('should trigger `toggle` action even with specified `value`' +
' argument', () => {
const ampSelector = getSelector({
config: {
count: 5,
selectedCount: 1,
},
});
ampSelector.build();
const impl = ampSelector.implementation_;

expect(ampSelector.hasAttribute('multiple')).to.be.false;
expect(ampSelector.children[0].hasAttribute('selected')).to.be.true;

// Test the case where the element to be `selected` and the currently
// selected element are different
let args = {'index': 2, 'value': true};
impl.executeAction(
{method: 'toggle', args, satisfiesTrust: () => true});
expect(ampSelector.children[0].hasAttribute('selected')).to.be.false;
expect(ampSelector.children[2].hasAttribute('selected')).to.be.true;

// Test the case where the element to be `selected` and the currently
// selected element are the same
args = {'index': 2, 'value': true};
impl.executeAction(
{method: 'toggle', args, satisfiesTrust: () => true});
expect(ampSelector.children[2].hasAttribute('selected')).to.be.true;

// Test the case where the element to be removed as `selected` and
// the currently selected element are the same
args = {'index': 2, 'value': false};
impl.executeAction(
{method: 'toggle', args, satisfiesTrust: () => true});
expect(ampSelector.children[2].hasAttribute('selected')).to.be.false;

// Test the case where the element to be removed as `selected`
// is different from the currently `selected` element
ampSelector.children[0].setAttribute('selected', '');
expect(ampSelector.children[0].hasAttribute('selected')).to.be.true;

args = {'index': 2, 'value': false};
impl.executeAction(
{method: 'toggle', args, satisfiesTrust: () => true});
expect(ampSelector.children[0].hasAttribute('selected')).to.be.true;
expect(ampSelector.children[2].hasAttribute('selected')).to.be.false;
});

it('should trigger `select` action when user selects an option', () => {
const ampSelector = getSelector({
config: {
Expand Down Expand Up @@ -740,7 +818,6 @@ describes.realWin('amp-selector', {
expect(ampSelector.children[1].hasAttribute('selected')).to.be.true;
});


describe('keyboard-select-mode', () => {

it('should have `none` mode by default', () => {
Expand Down
4 changes: 4 additions & 0 deletions spec/amp-actions-and-events.md
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,10 @@ event.response</pre></td>
<td><code>selectDown(delta=INTEGER)</code></td>
<td>Moves the selection down by the value of `delta`. The default `delta` is set to -1.</td>
</tr>
<tr>
<td><code>toggle(index=INTEGER, value=BOOLEAN)</code></td>
<td>Sets the selected element's `selected` attribute if value is 'true', otherwise removes the attribute</td>
</tr>
</table>

### amp-sidebar
Expand Down