-
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
Conversation
Hi @cvializ, Wanted to get some initial feedback re:the 2 questions in the PR. |
If Cool, I can see the number of items to skip being a useful optional parameter. Feel free to add it if you think it'll improve the usefulness of the component |
f58b751
to
524a18a
Compare
@@ -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 => { |
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 function select
?
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 is skip=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 to select_()
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.
@cvializ could you TAL? I have made the changes as per the discussion. Also filed a seperate issue for the toggling the select
attribute on an element
@@ -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 => { |
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 is skip=1
might imply moving and skipping over an option, instead of moving to it.
|
||
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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 comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can use the unary -
operator: -args['incrementPos']
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.
Done
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I would stick to const
s here to eliminate branching and mutations.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
And what if the user passes a negative incrementPos
that is over twice the length of the options? We may still need to modulo the negative value.
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.
- Done.
- Good catch! Made the change
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshed: maybe rename this delta
, since it could be an increment or a decrement
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.
Done. I am renaming the select{Up,Down} argument to delta
too. I like it.
spec/amp-actions-and-events.md
Outdated
</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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
@cvializ made the changes you asked for, PTAL?
@@ -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 => { |
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 to select_()
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I am renaming the select{Up,Down} argument to delta
too. I like it.
spec/amp-actions-and-events.md
Outdated
</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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
- Done.
- Good catch! Made the change
spec/amp-actions-and-events.md
Outdated
<td>Changes the selected element to the currentPos - `delta`. The default `delta` is set to 1.</td> | ||
</tr> | ||
<tr> | ||
<td><code>selectDown(decrementPos=INTEGER)</code></td> |
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.
nit: replace decrementPos
with delta
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.
Done
spec/amp-actions-and-events.md
Outdated
</tr> | ||
<tr> | ||
<td><code>selectUp(delta=INTEGER)</code></td> | ||
<td>Changes the selected element to the currentPos - `delta`. The default `delta` is set to 1.</td> |
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.
currentPos isn't a variable so it seems kind of out of context. Consider changing the wording to something like "Moves the selection up by delta
"
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.
Done
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Will this still work if the user calls myAmpSelector.selectDown(-1000)
?
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This could be a good utility method to add to src/math.js
since I'm sure it'd be generally useful.
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.
Done. Also added tests to test-math.js
which helped me catch another issue in the code.
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.
@cvializ PTAL? Thanks!
spec/amp-actions-and-events.md
Outdated
<td>Changes the selected element to the currentPos - `delta`. The default `delta` is set to 1.</td> | ||
</tr> | ||
<tr> | ||
<td><code>selectDown(decrementPos=INTEGER)</code></td> |
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.
Done
spec/amp-actions-and-events.md
Outdated
</tr> | ||
<tr> | ||
<td><code>selectUp(delta=INTEGER)</code></td> | ||
<td>Changes the selected element to the currentPos - `delta`. The default `delta` is set to 1.</td> |
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.
Done
// 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This could be a good utility method to add to src/math.js
since I'm sure it'd be generally useful.
((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 comment
The reason will be displayed to describe this comment to others. Learn more.
With the true modulo change, is it mathematically possible that normalizedIndex
could ever be negative? I'm not sure this user assert is necessary.
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.
Done.
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.
@cvializ PTAL? Thanks!
((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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also added tests to test-math.js
which helped me catch another issue in the code.
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.
Double LGTM!
@cvializ feel free to merge since the checks are passing! |
* WIP * Finalize implementation and tests * Make cvializ@ suggestions - change input argument name and clean up code * Cater for large negative numbers and add tests to check for that too * Move mod functionality to helper and add tests too
Fixes #10947
This PR:
<amp-selector>
elements which change the option selected by 1, however don't shift focus to that element.<amp-selector>
elements that is copied from the validator page.Things to consider:
multiple
attribute is set on<amp-selector>
elements, should we change the first selection or not handle the case?