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

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

Merged
merged 17 commits into from
Feb 26, 2018
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6ddfed6
WIP
nainar Feb 20, 2018
524a18a
Finalize implementation and tests
nainar Feb 20, 2018
bf28c8d
Implementation and tests
nainar Feb 20, 2018
2d3bd60
Make cvializ@ suggestions - change input argument name and clean up code
nainar Feb 21, 2018
33f521e
Cater for large negative numbers and add tests to check for that too
nainar Feb 21, 2018
4ec3718
Merge upstream
nainar Feb 21, 2018
bfd6e11
Rewrite code and tests to be cleaner
nainar Feb 21, 2018
a848f21
Move mod functionality to helper and add tests too
nainar Feb 21, 2018
6c8031d
Merge branch 'amp_selector_actions' into amp_selector_toggle
nainar Feb 21, 2018
29b52d8
Merge branch 'master' of github.com:ampproject/amphtml into amp_selec…
nainar Feb 21, 2018
e51d475
Merge branch 'amp_selector_actions' into amp_selector_toggle
nainar Feb 21, 2018
861554f
Remove unnecessary attribute logic
nainar Feb 22, 2018
79f93c7
Merge branch 'master' of github.com:ampproject/amphtml into amp_selec…
nainar Feb 22, 2018
6f988ae
Merge branch 'amp_selector_actions' into amp_selector_toggle
nainar Feb 22, 2018
d043d7f
Add assert to ensure index is > 0
nainar Feb 23, 2018
c6c85cb
Add additional assert
nainar Feb 24, 2018
0cb7e6b
Linter error
nainar Feb 24, 2018
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
70 changes: 70 additions & 0 deletions extensions/amp-selector/0.1/amp-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,28 @@ export class AmpSelector extends AMP.BaseElement {
this.element.addEventListener('click', this.clickHandler_.bind(this));
this.element.addEventListener('keydown', this.keyDownHandler_.bind(this));
}

this.registerAction('selectUp', invocation => {
const args = invocation.args;
const delta = (args && args['delta'] !== undefined) ? -args['delta'] : -1;
this.select_(delta);
}, ActionTrust.LOW);

this.registerAction('selectDown', invocation => {
const args = invocation.args;
const delta = (args && args['delta'] !== undefined) ? args['delta'] : 1;
this.select_(delta);
}, ActionTrust.LOW);

this.registerAction('toggle', invocation => {
const args = invocation.args;
if (args && args['index'] !== undefined) {
this.toggle_(args['index'],
args['value'] !== undefined ?
args['value'] :
!this.options_[args['index']].hasAttribute('selected'));
}
}, ActionTrust.LOW);
}

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

/**
* Handles toggle action.
* @param {number} index
* @param {boolean} value
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically for toggle functions the value parameter is optional. This will help eliminate the branches in the action callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can base your toggle implementation on the toggleExperiments function in src/experiments.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I was thinking that this method would have @param {boolean=} opt_value, then in the registerAction callback just call this.toggle_(args['index'], args['value']) since you're checking hasAttribute and if value is undefined twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup that makes sense. Made the change.

*/
toggle_(index, 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

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

// There is a change of the `selected` attribute for the element
if (indexFinalStatus !== indexCurrentStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we use shortcut if statements to avoid nested ifs, like

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

if (selectedIndex !== index) { /*...*/ } else { /*...*/ }

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.

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
*/
select_(delta) {
// 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 previousIndex = this.options_.indexOf(this.selectedOptions_[0]);
const index = previousIndex + delta;
const normalizedIndex = index > 0 ?
index % this.options_.length :
((index % this.options_.length) +
this.options_.length) % this.options_.length;

user().assert(normalizedIndex >= 0,
'This should have been positive');

this.setSelection_(this.options_[normalizedIndex]);
this.clearSelection_(this.options_[previousIndex]);
}

/**
* Handles keyboard events.
* @param {!Event} event
Expand Down
167 changes: 167 additions & 0 deletions extensions/amp-selector/0.1/test/test-amp-selector.js
View file
Open in desktop
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 All @@ -651,6 +729,95 @@ describes.realWin('amp-selector', {
ampSelector, 'select', /* CustomEvent */ eventMatcher);
});

it('should trigger `select` action when user uses ' +
'`selectUp`/`selectDown` action with default delta value of 1', () => {
const ampSelector = getSelector({
attributes: {
id: 'ampSelector',
},
config: {
count: 6,
},
});
ampSelector.children[0].setAttribute('selected', '');
ampSelector.build();
const impl = ampSelector.implementation_;

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

impl.executeAction({method: 'selectDown', satisfiesTrust: () => true});
expect(ampSelector.children[0].hasAttribute('selected')).to.be.false;
expect(ampSelector.children[1].hasAttribute('selected')).to.be.true;

impl.executeAction({method: 'selectUp', satisfiesTrust: () => true});

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

});

it('should trigger `select` action when user uses ' +
'`selectUp`/`selectDown` action with user specified delta value', () => {
const ampSelector = getSelector({
attributes: {
id: 'ampSelector',
},
config: {
count: 6,
},
});
ampSelector.children[0].setAttribute('selected', '');
ampSelector.build();
const impl = ampSelector.implementation_;

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

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

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

it('should trigger `select` action when user uses ' +
'`selectUp`/`selectDown` action with user specified delta value ' +
'(test large values)', () => {
const ampSelector = getSelector({
attributes: {
id: 'ampSelector',
},
config: {
count: 5,
},
});
ampSelector.children[1].setAttribute('selected', '');
ampSelector.build();
const impl = ampSelector.implementation_;

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

let args = {'delta': 1001};
impl.executeAction(
{method: 'selectDown', args, satisfiesTrust: () => true});
expect(ampSelector.children[1].hasAttribute('selected')).to.be.false;
expect(ampSelector.children[2].hasAttribute('selected')).to.be.true;

args = {'delta': 1001};
impl.executeAction(
{method: 'selectUp', args, satisfiesTrust: () => true});
expect(ampSelector.children[2].hasAttribute('selected')).to.be.false;
expect(ampSelector.children[1].hasAttribute('selected')).to.be.true;
});

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

it('should have `none` mode by default', () => {
Expand Down
20 changes: 20 additions & 0 deletions spec/amp-actions-and-events.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,26 @@ event.response</pre></td>
</tr>
</table>

### amp-selector
<table>
<tr>
<th>Action</th>
<th>Description</th>
</tr>
<tr>
<td><code>selectUp(delta=INTEGER)</code></td>
<td>Moves the selection up by the value of `delta`. The default `delta` is set to 1.</td>
</tr>
<tr>
<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
<table>
<tr>
Expand Down