feat(accordion): add two variations of html markup

[christiemolloy]

closes #1913

[patternfly-build]

Deploy preview for pf-next ready!

Built with commit 45b04ad

https://deploy-preview-1990--pf-next.netlify.com

[mattnolting]

Great work, I'd just add commas to documentation.

[mattnolting]

Suggested change

	| `.pf-c-accordion` | `<div>` `<dl>` | Initiates an accordion component. **Required**|
	| `.pf-c-accordion` | `<div>`, `<dl>` | Initiates an accordion component. **Required**|

[mattnolting]

Suggested change

	| `.pf-c-accordion__toggle` | `<h3><button>` `<dt><button>` | Initiates a toggle in the accordion. **Required** |
	| `.pf-c-accordion__toggle` | `<h3><button>`, `<dt><button>` | Initiates a toggle in the accordion. **Required** |

[mattnolting]

Suggested change

	| `.pf-c-accordion__expanded-content` | `<div>` `<dd>` | Initiates expanded content. **Must be paired with a button** |
	| `.pf-c-accordion__expanded-content` | `<div>`, `<dd>` | Initiates expanded content. **Must be paired with a button** |

[mattnolting]

Lgtm 👍

[jgiardino]

Thanks for updating this!!

Off-thread, @christiemolloy had asked about what the default variation should be, between the one with definition list and one with headings. My understanding with the react component is that if headingLevel is not included, then the definition list variation would be used. If headingLevel is specified, then we use the variation with headings and use the heading level specified in that prop. In that case, I think the variation with definition list would be considered the default, at least in the context of react. In the context of core, do we have a concept of default? Do our defaults in react mean anything in the context of core? I'm a little inclined to say that what's more important is we match examples and example order between the two. And the first react example doesn't have to be the variant that uses defaults.

I'm curious what others think, but in this example, I think we can leave the order as it is, and have the react examples updated to match the examples shown in core.

Other than that question, there's one minor issue I noticed when I expanded the code for the preview of the <dl> variation, where an extra </dt> is included:

  <dt>
    <button class="pf-c-accordion__toggle" aria-expanded="false">
      <span class="pf-c-accordion__toggle-text">
   Item one</span>
      <i class="fas fa-angle-right pf-c-accordion__toggle-icon" aria-hidden="true"></i>
    </button>
    </h3>
  </dt>
  </dt> <<<<<< EXTRA </dt> HERE
  <dd class="pf-c-accordion__expanded-content" hidden>
    <div class="pf-c-accordion__expanded-content-body">
      This text is hidden
    </div>
  </dd>

And a similar issue occurs in the example that uses <h3>:

<div class="pf-c-accordion">
  <h3>
    <button class="pf-c-accordion__toggle" aria-expanded="false">
      <span class="pf-c-accordion__toggle-text">
   Item one</span>
      <i class="fas fa-angle-right pf-c-accordion__toggle-icon" aria-hidden="true"></i>
    </button>
  </h3>
  </dt>   <<<<<< EXTRA </dt> HERE
  </h3>   <<<<<< EXTRA </h3> HERE
  <div class="pf-c-accordion__expanded-content" hidden>
    <div class="pf-c-accordion__expanded-content-body">
      This text is hidden
    </div>
  </div>

[jgiardino]

Related to my previous comment, I'm not sure that we want to label anything in core as a default, since the concept of defaults doesn't really exist here the way they do in the react repo. In react, there are defaults based on empty props, and for this component, I think the default variation would be the definition list variation.

Here, I would just say that one variation uses definition list and another uses headings. And also that we are using <h3> but really the heading levels that you use should fit within the rest of the heading outline on your page.

[jgiardino]

Since this is the only <button> in the accordion, maybe we can change this:
<h3><button>, <dt><button>
to just be this:
<button>

One benefit of this is not having to explain that the <h3> should really be a heading level that fits the rest of the heading outline on your page.

[christiemolloy]

per our offline conversation, updating this to be <h1 - h6>

[christiemolloy]

Updated @jgiardino

[jgiardino]

Everything looks good! 🎉 Thanks!

[patternfly-build]

🎉 This PR is included in version 2.16.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀