Add expand-single-section attribute to <amp-accordion>. #13364

nainar · 2018-02-08T04:10:05Z

This PR does the following

Adds an expand-one attribute to <amp-accordion> that ensures that only one <section> in <amp-accordion> is expanded at any given time.
Adds a test for the expansion/collapse of <section>s in
Adds an example file (I found it weird that <amp-accordion> didn't have an example page. Is that for a reason?). This example needs some work.

…ccordion.

aghassemi · 2018-02-08T19:25:11Z

examples/accordion.amp.html

+  <script async custom-element="amp-accordion" src="https://cdn.ampproject.org/v0/amp-accordion-0.1.js"></script>
+</head>
+<body>
+  <amp-accordion expand-one>


let's think of the name a bit more

I recommend single as the attribute

how about expand-single-section?

sgtm, descriptive.

Change made in latest commit.

cvializ

Nice, just one question before LGTM

cvializ · 2018-02-08T19:26:19Z

extensions/amp-accordion/0.1/amp-accordion.js

      if (toExpand) {
        section.setAttribute('expanded', '');
        header.setAttribute('aria-expanded', 'true');
+        // if expand-one is set, only allow one <section> to be expanded at a time
+        if (this.element.hasAttribute('expand-one')) {
+          this.sections_ = this.getRealChildren();


Is it necessary to reassign this.sections_? Will it be different than the this.sections_ assigned in buildCallback()?

Nope, you are right. Ommitted

cvializ · 2018-02-08T19:27:47Z

extensions/amp-accordion/amp-accordion.md

 #### `toggle`
 This action toggles between the `expanded` and `collapsed` states of the `amp-accordion`. When called with no arguements, it will toggle all sections of the accordion. A single section may be specified with the `section` arguement and the corresponding `id` as the value.
+


Nice, the spacing looks better 👍

cvializ · 2018-02-08T19:31:32Z

extensions/amp-accordion/0.1/test/test-amp-accordion.js

+      expect(ampAccordion.hasAttribute('expand-one')).to.be.true;
+      const impl = ampAccordion.implementation_;
+      const headerElements = doc.querySelectorAll(
+          'section > *:first-child');


note: I see this in the rest of the file so you don't have to change it, but technically the * is not required and it could just be section > :first-child

Yup, thanks for bringing this to my attention

nainar

@cvializ made the change you asked for.

@aghassemi have a name suggestion for you, PTAL?

nainar · 2018-02-08T22:58:21Z

examples/accordion.amp.html

+  <script async custom-element="amp-accordion" src="https://cdn.ampproject.org/v0/amp-accordion-0.1.js"></script>
+</head>
+<body>
+  <amp-accordion expand-one>


how about expand-single-section?

nainar · 2018-02-08T22:59:44Z

extensions/amp-accordion/0.1/amp-accordion.js

      if (toExpand) {
        section.setAttribute('expanded', '');
        header.setAttribute('aria-expanded', 'true');
+        // if expand-one is set, only allow one <section> to be expanded at a time
+        if (this.element.hasAttribute('expand-one')) {
+          this.sections_ = this.getRealChildren();


Nope, you are right. Ommitted

nainar · 2018-02-08T23:02:21Z

extensions/amp-accordion/0.1/test/test-amp-accordion.js

+      expect(ampAccordion.hasAttribute('expand-one')).to.be.true;
+      const impl = ampAccordion.implementation_;
+      const headerElements = doc.querySelectorAll(
+          'section > *:first-child');


Yup, thanks for bringing this to my attention

nainar · 2018-02-09T01:11:48Z

@cvializ / @aghassemi for review

aghassemi · 2018-02-09T01:16:03Z

extensions/amp-accordion/amp-accordion.md

@@ -86,6 +89,10 @@ Set this attribute on the `<amp-accordion>` to opt out of preserving the collaps

 Set this attribute on a `<section>` to display the section as expanded on page load.

+#####  expand-single-section


please add ticks ` around the attribute name

Done and did this for the other 2 attributes as well.

nainar · 2018-02-12T02:45:22Z

Hi @cvializ could you merge if this looks good to you? Thanks!

cvializ · 2018-02-16T15:30:45Z

Looks good, thanks @nainar!

…3364) * Add expand-one attribute to amp-accordion. Add example file for amp-accordion. * Address cvializ@'s comments * Rename expand-one attribute to expand-single-section * Add ticks around attribute names

ampproject#13386, and ampproject#13625

* version bump and experimental * Remove '(CDATA)' from the error messages. Just say 'text'. * Revision bump for #13364, #13500, #13536, #13386, and #13625

…3364) * Add expand-one attribute to amp-accordion. Add example file for amp-accordion. * Address cvializ@'s comments * Rename expand-one attribute to expand-single-section * Add ticks around attribute names

* version bump and experimental * Remove '(CDATA)' from the error messages. Just say 'text'. * Revision bump for ampproject#13364, ampproject#13500, ampproject#13536, ampproject#13386, and ampproject#13625

…3364) * Add expand-one attribute to amp-accordion. Add example file for amp-accordion. * Address cvializ@'s comments * Rename expand-one attribute to expand-single-section * Add ticks around attribute names

Add expand-one attribute to amp-accordion. Add example file for amp-a…

28685c7

…ccordion.

googlebot added the cla: yes label Feb 8, 2018

aghassemi requested a review from cvializ February 8, 2018 19:06

aghassemi reviewed Feb 8, 2018

View reviewed changes

cvializ reviewed Feb 8, 2018

View reviewed changes

Address cvializ@'s comments

c77e6bb

nainar commented Feb 8, 2018

View reviewed changes

nainar changed the title ~~Add expand-one attribute to <amp-accordion>.~~ Add expand-single-section attribute to <amp-accordion>. Feb 9, 2018

Rename expand-one attribute to expand-single-section

47fce58

aghassemi approved these changes Feb 9, 2018

View reviewed changes

Add ticks around attribute names

da4a1b3

cvializ merged commit 289ef16 into ampproject:master Feb 16, 2018

This was referenced Feb 22, 2018

Status Update 2018-02-19 #13549

Closed

Feature request: amp-accordion to add a flag so it only expand one section at a time. #6737

Closed

powdercloud pushed a commit to powdercloud/amphtml that referenced this pull request Feb 27, 2018

Revision bump for ampproject#13364, ampproject#13500, ampproject#13536,

3e589d7

ampproject#13386, and ampproject#13625

powdercloud added a commit that referenced this pull request Feb 28, 2018

Propagate Github Changes (#13702)

8e63b40

* version bump and experimental * Remove '(CDATA)' from the error messages. Just say 'text'. * Revision bump for #13364, #13500, #13536, #13386, and #13625

aghassemi mentioned this pull request Mar 13, 2018

Why is it wrong that the attribute 'expand-single-section' may not appear in tag 'amp-accordion'? #13948

Closed

cathyxz mentioned this pull request Jun 12, 2018

Add examples for amp-accordion's new attributes ampproject/amp-by-example#1307

Merged

nainar mentioned this pull request Jul 16, 2018

Close all other sidebars when a new sidebar is opened. #3320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expand-single-section attribute to <amp-accordion>. #13364

Add expand-single-section attribute to <amp-accordion>. #13364

nainar commented Feb 8, 2018

aghassemi Feb 8, 2018

nainar Feb 8, 2018

aghassemi Feb 9, 2018

nainar Feb 9, 2018

cvializ left a comment

cvializ Feb 8, 2018

nainar Feb 8, 2018

cvializ Feb 8, 2018

cvializ Feb 8, 2018

nainar Feb 8, 2018

nainar left a comment

nainar Feb 8, 2018

nainar Feb 8, 2018

nainar Feb 8, 2018

nainar commented Feb 9, 2018

aghassemi Feb 9, 2018

nainar Feb 9, 2018

nainar commented Feb 12, 2018

cvializ commented Feb 16, 2018

		#### `toggle`
		This action toggles between the `expanded` and `collapsed` states of the `amp-accordion`. When called with no arguements, it will toggle all sections of the accordion. A single section may be specified with the `section` arguement and the corresponding `id` as the value.

		@@ -86,6 +89,10 @@ Set this attribute on the `<amp-accordion>` to opt out of preserving the collaps

		Set this attribute on a `<section>` to display the section as expanded on page load.

		##### expand-single-section

Add expand-single-section attribute to <amp-accordion>. #13364

Add expand-single-section attribute to <amp-accordion>. #13364

Conversation

nainar commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nainar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nainar commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nainar commented Feb 12, 2018

cvializ commented Feb 16, 2018