-
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> element toggle the 'selected' attribute #13553
Conversation
@cvializ could you TAL? Also sorry for the PRs, feel free to distribute amongst others on the team as needed! :) |
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.
Hey it's no problem, will do : )
/** | ||
* Handles toggle action. | ||
* @param {number} index | ||
* @param {boolean} 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.
Typically for toggle functions the value
parameter is optional. This will help eliminate the branches in the action callback.
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.
You can base your toggle implementation on the toggleExperiments
function in src/experiments.js
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 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.
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.
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.
Yup that makes sense. Made the change.
// past the beginning or end. | ||
const selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); | ||
|
||
if (value == true) { |
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 simplify this to if (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.
Changed 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.
Rewrote the code to be cleaner hopefully, PTAL? Thanks!
/** | ||
* Handles toggle action. | ||
* @param {number} index | ||
* @param {boolean} 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.
Changed the code.
// past the beginning or end. | ||
const selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); | ||
|
||
if (value == true) { |
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 the code.
// 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 = |
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.
The !!
shouldn't be necessary here, since value and indexCurrentStatus are already boolean.
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
const selectedIndex = this.options_.indexOf(this.selectedOptions_[0]); | ||
|
||
// There is a change of the `selected` attribute for the element | ||
if (indexFinalStatus !== indexCurrentStatus) { |
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.
Usually we use shortcut if
statements to avoid nested if
s, like
if (indexFinalStatus === indexCurrentStatus) {
return;
}
if (selectedIndex !== index) { /*...*/ } else { /*...*/ }
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.
/** | ||
* Handles toggle action. | ||
* @param {number} index | ||
* @param {boolean} 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.
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.
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?
/** | ||
* Handles toggle action. | ||
* @param {number} index | ||
* @param {boolean} 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.
Yup that makes sense. Made the change.
// 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 = |
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
const selectedIndex = this.options_.indexOf(this.selectedOptions_[0]); | ||
|
||
// There is a change of the `selected` attribute for the element | ||
if (indexFinalStatus !== indexCurrentStatus) { |
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('toggle', invocation => { | ||
const args = invocation.args; | ||
if (args && args['index'] !== undefined) { | ||
this.toggle_(args['index'], args['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.
Let's do a user assert to make sure the index
is valid
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.
Added an assert to make sure that the index
is > 0, could also add one to make sure it is less than length of the options
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 think that both bounds would be helpful. If it ends up being too noisy we can downgrade it to a user().warn()
later.
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 is an assert for index
>=0 enough or do you want one for <= length of options too?
this.registerAction('toggle', invocation => { | ||
const args = invocation.args; | ||
if (args && args['index'] !== undefined) { | ||
this.toggle_(args['index'], args['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.
Added an assert to make sure that the index
is > 0, could also add one to make sure it is less than length of the options
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 ' + |
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.
Added the aditional assert.
@cvializ added the additional assert. PTAL? |
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
) * WIP * Finalize implementation and tests * 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 * Rewrite code and tests to be cleaner * Move mod functionality to helper and add tests too * Remove unnecessary attribute logic * Add assert to ensure index is > 0 * Add additional assert * Linter error
Fixes #13550 by adding the ability to toggle the
selected
attribute on an element inside<amp-selector>
The attribute has two attributes:
selected
attribute.E.g.:
<button on="tap:mySelector.toggle(index=3, value=true)">Select the third option</button>
P.S. The parent of this branch was #13439. Will rebase once that lands.