-
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
<amp-selector> Add select{Up/Down} actions #13439
Changes from 2 commits
6ddfed6
524a18a
2d3bd60
33f521e
a848f21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,6 +113,24 @@ 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; | ||
if (args && args['incrementPos'] !== undefined) { | ||
this.select_(-1 * args['incrementPos']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: you can use the unary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} else { | ||
this.select_(-1); | ||
} | ||
}, ActionTrust.LOW); | ||
|
||
this.registerAction('selectDown', invocation => { | ||
const args = invocation.args; | ||
if (args && args['decrementPos'] !== undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this could be simplified to eliminate the branch. const decrementPos = (args && args['decrementPos'] !== undefined) ?
args['decrementPos'] : 1;
this.select_(decrementPos); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
this.select_(args['decrementPos']); | ||
} else { | ||
this.select_(1); | ||
} | ||
}, ActionTrust.LOW); | ||
} | ||
|
||
/** @override */ | ||
|
@@ -319,6 +337,27 @@ export class AmpSelector extends AMP.BaseElement { | |
} | ||
} | ||
|
||
/** | ||
* Handles selectUp events. | ||
* @param {number} incrementPos | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bikeshed: maybe rename this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I am renaming the select{Up,Down} argument to |
||
*/ | ||
select_(incrementPos) { | ||
// 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. | ||
let selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would stick to const previousIndex = this.options_.indexOf(this.selectedOptions_[0]);
const index = previousIndex + incrementPos;
const normalizedIndex = index < 0 ? index + this.options_.length : index % this.options_.length; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what if the user passes a negative There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const oldSelectedIndex_ = selectedIndex_; | ||
|
||
selectedIndex_ = (selectedIndex_ + incrementPos) % this.options_.length; | ||
if (selectedIndex_ < 0) { | ||
selectedIndex_ = selectedIndex_ + this.options_.length; | ||
} | ||
|
||
const selectedOption = this.options_[selectedIndex_]; | ||
this.setSelection_(selectedOption); | ||
this.clearSelection_(this.options_[oldSelectedIndex_]); | ||
} | ||
|
||
/** | ||
* Handles keyboard events. | ||
* @param {!Event} event | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -362,6 +362,22 @@ event.response</pre></td> | |
</tr> | ||
</table> | ||
|
||
### amp-selector | ||
<table> | ||
<tr> | ||
<th>Action</th> | ||
<th>Description</th> | ||
</tr> | ||
<tr> | ||
<td><code>selectUp(incrementPos=INTEGER)</code></td> | ||
<td>Changes the selected element to the currentPos-incrementPos. If no increment value is specified, the default is 1.</td> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: put backticks `` around variable names in docs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
</tr> | ||
<tr> | ||
<td><code>selectDown(decrementPos=INTEGER)</code></td> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
<td>Changes the selected element to the currentPos+decrementPos. If no decrement value is specified, the default is -1.</td> | ||
</tr> | ||
</table> | ||
|
||
### amp-sidebar | ||
<table> | ||
<tr> | ||
|
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.
Bikeshed naming - should the attribute be changed to
skip
and then the dev can pass in negative or positive values to the functionselect
?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.
maybe
count
?skip
could be confusing, like isskip=1
might imply moving and skipping over an option, instead of moving to it.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.
Changed this to
delta
since I thought it best reflected the attribute and matched up with your suggestion for the input argument toselect_()