setPanelHeight should account for margins of inner elements.

[kotyy]

Expected Behavior

setPanelHeight should account for margins of inner elements.

Current Behavior

setPanelHeight does not account for margins of inner elements.

This may be an opinionated position that you disagree with, but out of the box, throwing a paragraph into .js-badger-accordion-panel-inner, the height calculation does not account for the paragraphs margin.

      <div class="js-badger-accordion-panel-inner">
        <p>more content</p>
      </div>

Three possible solutions:

1. Edit the setPanelHeight to include margin in its calculation. This could be optional.
2. Add another element inside of .js-badger-accordion-panel-inner styled inline-block with a width of 100%.
3. Documenting that the height calculation doesn't include margin.

Digging the plugin so far, so thanks for sharing it with the world!

[stuartjnelson]

Hi @kotyy,

Thanks for getting in touch! I've been puzzling over this for where the responsibility should lie and I don't think out of the box it should be with the accordion. I think all your ideas make sense. I want to keep the necessary markup as minimal as possible so don't want to go down that route.

Adding in a note to the docs that the default calculation does not include margin is an awesome idea! I think adding an option to let the setPanelHeight also take into account margin would be cool. I can then collect feedback and possible make that the default in the future. How does that sound?

If you fancy starting it off and making a PR feel free! I won't be able to action this myself for a few weeks as I am slammed with paid work. I will get to this. I will action adding the notes to the docs first then get to adding an optional change to the height calulation after. There are a few other issues I want to get to before that but I will get to it.

Thanks again for your suggestions. Are you using the accordion for a project you'd be happy to share? I am keen to get some examples together of sites using the accordion as a showcase. Let me know if you're interested.

[kotyy]

I'm not very familiar with ES6 classes, to be honest, but it looks like the best pattern would be to extend the BadgerAccordion class and just override the method.

I've fixed it on this current project using CSS, but after I get this out the door I might be able to make a PR with a doc update demonstrating this.

This is the first time I've used this project. Previously I've been using jQuery Accessible Accordion, but I'm finally getting around to dropping jQuery from my toolset. Once the project launches, I'll try to send you a link.