-
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 4 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,18 @@ 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); | ||
} | ||
|
||
/** @override */ | ||
|
@@ -319,6 +331,28 @@ export class AmpSelector extends AMP.BaseElement { | |
} | ||
} | ||
|
||
/** | ||
* 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]); | ||
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. Will this still work if the user calls 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. TIL: Javascript % is not true modulo. Adapted the answer here to work: https://stackoverflow.com/a/47354356 I have also added tests to check select{Up,Down}(1001) 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. Nice! This could be a good utility method to add to 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. Also added tests to |
||
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, | ||
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. With the true modulo change, is it mathematically possible that 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 should have been positive'); | ||
|
||
this.setSelection_(this.options_[normalizedIndex]); | ||
this.clearSelection_(this.options_[previousIndex]); | ||
} | ||
|
||
/** | ||
* Handles keyboard events. | ||
* @param {!Event} 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.
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_()