Add expand, collapse, toggle actions to amp-accordion
#11933
Conversation
|
Whoops, I'll correct the issues found by Travis CI |
| @@ -59,6 +59,40 @@ class AmpAccordion extends AMP.BaseElement { | |||
| this.sessionId_ = this.getSessionStorageKey_(); | |||
| this.currentState_ = this.getSessionState_(); | |||
|
|
|||
| this.registerAction('toggle', invocation => { | |||
| if (invocation.args) { | |||
| const sectionEl = document.getElementById(invocation.args['section']); | |||
There was a problem hiding this comment.
please use this.getAmpDoc().getElementById()
| this.registerAction('toggle', invocation => { | ||
| if (invocation.args) { | ||
| const sectionEl = document.getElementById(invocation.args['section']); | ||
| this.toggle_(sectionEl); |
There was a problem hiding this comment.
let's do a
user().assertElement(
sectionEl,
'No element found with id:' + sectionId);
so devs can see what's happening if they make a mistake
| @@ -59,6 +59,40 @@ class AmpAccordion extends AMP.BaseElement { | |||
| this.sessionId_ = this.getSessionStorageKey_(); | |||
| this.currentState_ = this.getSessionState_(); | |||
|
|
|||
| this.registerAction('toggle', invocation => { | |||
| if (invocation.args) { | |||
| const sectionEl = document.getElementById(invocation.args['section']); | |||
There was a problem hiding this comment.
let's also put invocation.args['section'] in a constant to reuse later (see next comment)
| this.toggle_(sectionEl); | ||
| } else { | ||
| const accordionEl = document.getElementById(invocation.target.id); | ||
| for (let i = 0; i < accordionEl.children.length; i++) { |
There was a problem hiding this comment.
buildCallback() queries for all sections already but keeps them in a local variable. Let;s make that an instance variable and reuse it here instead of doing accordionEl.children
| const content = sectionComponents[1]; | ||
| const contentId = content.getAttribute('id'); | ||
| const isSectionClosedAfterClick = section.hasAttribute('expanded'); | ||
| this.mutateElement(() => { | ||
| if (section.hasAttribute('expanded')) { | ||
| if (typeof opt_forceExpand !== 'undefined' && opt_forceExpand) { |
There was a problem hiding this comment.
let's simplify this with something like
let toExpand;
if(opt_forceExpand === true) {
toExpand = true;
} else if( opt_forceExpand === false) {
toExpand = false;
} else {
toExpnd = section.hasAttribute('expanded')
}
if(toExpand) {
...
} else {
..
}
|
Thanks for the PR @mianuddin, left some comments. Nothing major. |
|
@mianuddin, friendly ping regarding the review comments. |
|
@aghassemi Sorry for not implementing the changes sooner, I just had a big midterm. I'm aiming to complete the changes by end of week. |
|
@mianuddin no rush, take your time. Just wanted to make sure still interested in continuing the work. |
|
@aghassemi Apologies for the late commits. I've made the changes requested and fixed as much as I could from Travis CI. Your comments were extremely helpful, I really appreciate it. |
|
@mianuddin can you please rebase (or send another PR if too tangled to fix). PR shows 259 files changed and tons of unrelated commits |
|
@aghassemi I was able to rebase it 👍 |
|
@mianuddin just a few lint errors causing tests to fail. |
|
@aghassemi I'll get on those, thanks! |
|
@aghassemi You're good to go! 💯 |
expand,collapse,toggleactions toamp-accordion#10826expand,collapse,toggleactions toamp-accordionReviewer: @aghassemi