amp-accordion #1849
amp-accordion #1849
Conversation
|
Please elaborate description a bit and reference issue(s). Done |
| if (event.target) { | ||
| var headerElements = | ||
| this.element.querySelectorAll('section > *:first-child'); | ||
| for(var i=0; i< headerElements.length; i++) { |
camelburrito
force-pushed
the
accordion
branch
from
5ae6c8b to
3b5efa1
Compare
| } | ||
| /** @override */ | ||
| isRelayoutNeeded() { | ||
| return true; |
There was a problem hiding this comment.
why isRelayoutNeeded? when we override this its usually because there's some work in layoutCallback
There was a problem hiding this comment.
Correct removing the the commit.
camelburrito
force-pushed
the
accordion
branch
2 times, most recently
from
69eeaed to
7562c40
Compare
|
|
||
| An accordion provides a way for viewers to have a glance at the outline of the content and jump to a section or their choice at their will. This would be extremely helpful for handheld mobile devices where even a couple of sentences in a section would lead to the viewer needing to scroll. | ||
|
|
||
| #### Behavior |
There was a problem hiding this comment.
Can you add something about where the expand/collapse behavior is triggered? I assume it's the first child of a <section>, which is considered the heading, but it wasn't clear to me from this doc.
camelburrito
force-pushed
the
accordion
branch
from
7562c40 to
c847de7
Compare
| /* Add expand and collapse signs to the heading */ | ||
| amp-accordion > section > *:first-child:after { | ||
| position: absolute !important; | ||
| content: '+'; |
There was a problem hiding this comment.
Do we have to have + defined right away? Maybe this should be left to the author?
There was a problem hiding this comment.
Agreed. Removed
camelburrito
force-pushed
the
accordion
branch
from
c847de7 to
0fcbb96
Compare
|
@sriramkrish85 , can you designate this as an experiment until we've gotten a chance to work with it and make sure it meets publisher's needs? This is a standalone extension - as long it delivers what it tells in the readme file it should be fine. Developers could try and use this and send in their feedback and we can build incrementally. |
camelburrito
force-pushed
the
accordion
branch
2 times, most recently
from
df5e3f4 to
3eb88e2
Compare
|
|
||
| /** @override */ | ||
| buildCallback() { | ||
| /** @const @private {!NodeList} */ |
There was a problem hiding this comment.
Let's add experiment as requested by @ericlindley-g
camelburrito
force-pushed
the
accordion
branch
3 times, most recently
from
921865d to
aad6fb1
Compare
|
PTAL |
| 'Each section must have exactly two children. ' + | ||
| 'See https://github.com/ampproject/amphtml/blob/master/extensions/' + | ||
| 'amp-accordion/amp-accordion.md. Found in: %s', this.element); | ||
| if (!this.isExperimentOn_) { |
There was a problem hiding this comment.
Why are you doing this in the #forEach loop?
camelburrito
force-pushed
the
accordion
branch
from
aad6fb1 to
1aae4ee
Compare
|
LGTM |
amp-accordion - experimental
An accordion provides a way for viewers to have a glance at the outline of the content and jump to a section or their choice at their will. This would be extremely helpful for handheld mobile devices where even a couple of sentences in a section would lead to the viewer needing to scroll.
Solves #1017 (without the async content loading)
Referencing #827 since Expando is a part of it's discussion.