Expected Behavior

The visible label / unique accessible name of each button should be what's announced to screen readers when buttons are focused.

Current Behavior

"Open accordion panel" is the accessible name of all accordion panel triggers.

Steps to Reproduce

1. Turn on a screen reader
2. Navigate to an accordion trigger
3. The aria-label overwrites the accessible name of the button.

Fix

The aria-labels should be removed from these buttons.

1. Because they're overwriting the accessible name of the button
2. They're unnecessary as aria-expanded already handles the appropriate announcement of state.

Missing one /

I have reduced the padding and font size in the panel header to fit my website onto a tablet screen (I need to fit a mjpeg stream and an open panel). Is there a way of reducing the icon size so it fits better in a header with reduced padding? Is the best way to resize all pixel values relating to the icon in the css file?

Many thanks,
The accordion is great,
Sam

Hello,

Thanks a lot for this great accordion, very easy to implement.
Here is a suggestion : could we add a BEM modifier on both header and panel ?

For instance, this would allow us to style the active header with

.badger-accordion__header--open {
  // ...
}

.badger-accordion__panel--open {
  // ...
}

I have seen that the class -ba-is-hidden is applied on the panel and can be set through options, but that would be nice to use the same behaviour here too.

What do you think about it ?

Hello!

I'm using npm package, but it throws TransformError because of babel. Please, add .babelrc to .npmignore.

You can look at similar problems in other projects:
michaelcontento/redux-storage#65
goatslacker/alt#558

Thanks!

Unless I'm mistaken, I think there's a bug with the openAll() and closeAll() methods.

Taking openAll() as an example:

  this.headers.forEach(function (header) {
   _this5.togglePanel('open', header);
  });

But togglePanel() is expecting the header index , rather than the object, as the second parameter.

This seems to work:

  this.headers.forEach(function (header, i) {
    _this5.togglePanel('open', i);
  });

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!

Prerequisites

• I am running the latest version
• I checked the documentation and found no answer
• I checked to make sure that this issue has not already been filed
• I'm reporting the issue to the correct repository (for multi-repository projects)

Expected Behavior

hiddenClass not working properly in 1.2.0
Should allow me to change class, but nothing changes. Surprisingly, hidenClass still works even though it's deprecated.

Current Behavior

Unchanged selectors.

What is the current behavior?
Nothing changes and options not working properly.

Steps to Reproduce

Step 1:
I changed my JS file to read:

hidenClass: 'is-hidden'

Hidden class becomes .is-hidden - which is good.

Step 2:
I changed my JS file to read:

hiddenClass: 'is-hidden'

Hidden class reverts to default -ba-is-hidden - which is bad.

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

• Operating System: MacOS
• Browser/s & version/s: Chrome Version 71.0.3578.98
• NPM version: 6.4.0
• Node version: v10.8.0

First and foremost, thanks for this! I also think it's great that this allows flexibility in the markup. However, I suggest either adding a role of header to the dt element (as in the WAI example), or better yet, dispense with all of the aria roles and go with a header-paragraph pattern (optionally making the latter a region). As it stands, the roles render the definition list as a generic container. This being the case, why not drop the definition list elements and just use a real header element plus either a generic block (e.g.,

) or optionally use the region element?

Hi,
Really love the Badger Accordion. Only issue I have is when I resize my window the size of my content gets longer as it gets narrower, thus pushing some content below the max-height of the container.
Hope that makes sense?
Thanks for all the work you have put into this,
Chris

Expected Behavior

After an event is loaded from the inner compoenent the calculateAllPanelsHeight function is called on the ref for the accordion and the height should be set to match the inner componeents height.

Current Behavior

The inner component loads and triggers the calculateAllPanelsHeight functiuon but the panel does not res-size correctley

Note this does not happen if the console is open at the bottom for some reason and also when i zoom out a little the problem goes away

Also digging into the code I started logging the active height it is trying to set when it has called the calculatePanelHeight and I see when it closes that it calcs the height correctley at 1057 but then when I open it gives the height of 961.

Currently, the <button> to open a panel is called header. I think this could be confused with the HTML element and should be renamed to trigger in the docs as well as any other properties (class, data-attr etc...).

@seanjhulse what do you think? I can't workout a clean way to do this without creating a breaking change. Any ideas/opinions would be most welcome!

Hi,

Is it possible to do nested accordions?

I implemented the solution below but clicking on the nested accordion header closes the parent.

Thanks!

   const elements = {
    accordions: document.querySelectorAll('.js-badger-accordion'),
  };

  Array.from(elements.accordions).forEach((accordion) => {
    //const badgerAccordion = new BadgerAccordion(`#${accordion.id}`);
    id = document.getElementById(accordion.id);
    const badgerAccordion = new BadgerAccordion(id);
  });

initalised → initialized
In the options and class name

Prerequisites

Please answer the following questions for yourself before submitting an issue. YOU MAY DELETE THE PREREQUISITES SECTION.

• I am running the latest version
• I checked the documentation and found no answer
• I checked to make sure that this issue has not already been filed
• I'm reporting the issue to the correct repository (for multi-repository projects)

Expected Behavior

Please describe the behavior you are expecting

Create multiple instances on the same page.

Current Behavior

What is the current behavior?

Currently you can only have one instance per page or you have to give each accordion an unique ID / selector.

Steps to Reproduce

Create multiple accordions in your HTML wich work independently of each other.

const elements = {
  accordions: document.querySelectorAll('.js-badger-accordion'),
};

Array.from(elements.accordions).forEach((accordion) => {
  const badgerAccordion = new BadgerAccordion(accordion);
});

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

• Operating System: macOS
• Browser/s & version/s: Chrome 65
• NPM version: 5.6
• Node version: 9.10

Solution

Instead of passing a CSS selector as element to the new instance, I suggest passing a DOM node to it:
https://github.com/stuartjnelson/badger-accordion/blob/master/src/js/badger-accordion.js#L19

Currently I give each accordion an unique id to work around this issue but this cost more DOM lookups than nessessary

const elements = {
  accordions: document.querySelectorAll('.js-badger-accordion'),
};

Array.from(elements.accordions).forEach((accordion) => {
  const badgerAccordion = new BadgerAccordion(`#${accordion.id}`);
});

Expected Behavior

Use .open() to open a specific panel and then I expect the user to be able to click that header to close it.

Current Behavior

Using .open() on load causes a few issues:

1. The panel cannot be closed now via clicking behavior.
2. When combined with the openMultiplePanels option, if the user clicks on a different header than the opened one it will close the panel that was previously opened with the .open() method.

Failure Information (for bugs)

I looked a bit at the code and it looks like for .open() vs the openHeadersOnLoad option use different internal methods to achieve their goal. .open() uses the toggle method and openHeadersOnLoad actually changes the state. I suspect they should both be affecting the state.

Steps to Reproduce

After initializing an accordion, use the .open() method to open a specific panel. Once rendered, try to click the header to close the open panel. It should not close nor throw any errors.

Context

• Operating System: Mac OSX Mojave 10.14
• Browser/s & version/s: Chrome 70.0.3538.77
• NPM version: 5.6.0
• Node version: 9.10.0

Hello, badger accordion prevents jquery scripts from running, like jquery modal (http://jquerymodal.com), mega-menu (https://codepen.io/riogrande/pen/MKXweV)

Great work on the accordion Stu. Fantastic to see it launched. Some constructive feedback

Env Vars

Add in env vars to distinguish between "dev" and "production" builds (ie: whether Rollup is building to aid you in development or whether you're building a final package for consumption by users).

import replace from 'rollup-plugin-replace';

...
export default {
  ...
  plugins: [
    ...
    replace({
      ENV: JSON.stringify(process.env.NODE_ENV || 'development'),
    }),
  ],
};

Then you can prepend whatever command you're using to run the rollup build with the correct ENV setting. Eg:

NODE_ENV=production rollup

Apply Plugins Conditionally

I notice you have uglify commented out. Once you have implemented the above then you can conditionally apply the plugins as follows

export default {
  ...
  plugins: [
    ...
    replace({
      ENV: JSON.stringify(process.env.NODE_ENV || 'development'),
    }),
    (process.env.NODE_ENV === 'production' && uglify()),
  ],
};

Use scripts

I notice there is no indication as to how you're running the rollup commands. You can use the scripts section (docs) of package.json for this.

For example,

"scripts": {
	"dev": "NODE_ENV=develop ./node_modules/.bin/rollup -c",
	"build": "NODE_ENV=production ./node_modules/.bin/rollup -c" // args and config would probably be different between dev and production
}

Use module field

Apparently you should add a module field to the package.json which points to your ES Module file. That will allow ES6-aware tools like Rollup and webpack 2 to import the ES6 module version directly. You are correct to point the main field to the bundled output version.

Array.from polyfill

In some ways you are right to not bundle the Array.from into your code. This avoids the small perf penalty people have to pay if they already have it polyfilled. That being said, not bundling it assumes people will read your docs...they may well not.

Given the size of the plugin I would suggest you just bundle it. It should be wrapped in a conditional (check the output matches something like this example) to ensure it's not applied if it's already polyfilled. If it becomes a problem you could always look to create a bundle with and without the polyfill. This would be fairly simple. You could just create x2 main files and have these import badger script, but include/excluding the polyfill before the import. This would produce x2 output files - one with and one without the polyfill. Just an idea.

Prerequisites

• [ x ] I am running the latest version
• [ x ] I checked the documentation and found no answer
• [ x ] I checked to make sure that this issue has not already been filed
• [ x ] I'm reporting the issue to the correct repository (for multi-repository projects)

Expected Behavior

I want this accordion to be a tab list when you reach the desktop breakpoint for better UX

Current Behavior

Currently you only have an accordion or not. Therefor a destroy method would be helpful.

Thank your for this plugin!

Hello,
There is an error when is-hidden class is already definied in css :

example :
https://codepen.io/erwanpoupard/pen/OvvvgL

The accordion doesnt work anymore.

Prerequisites

Please answer the following questions for yourself before submitting an issue. YOU MAY DELETE THE PREREQUISITES SECTION.

• I am running the latest version
• I checked the documentation and found no answer
• I checked to make sure that this issue has not already been filed
• I'm reporting the issue to the correct repository (for multi-repository projects)

Expected Behavior

Please describe the behavior you are expecting

Current Behavior

What is the current behavior?

Failure Information (for bugs)

Please help provide information about the failure if this is a bug. If it is not a bug, please remove the rest of this template.

Steps to Reproduce

Please provide detailed steps for reproducing the issue.

1. step 1
2. step 2
3. you get it...

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

• Operating System:
• Browser/s & version/s:
• NPM version:
• Node version:

Failure Logs

Please include any relevant log snippets or files here.

Hi there! Thanks for creating this awesome, easy-to-use, and accessible accordion component! Extremely handy.

One major issue though that would prevent me from actually using this in a project is the lack of an "unmount" method. Most of the sites I build use a PJAX library such as Barba to create fluid transition animations between pages. In order to avoid memory leaks, I would need some way to "unmount" badger accordion (i.e. remove event listeners) before navigating to a new page. I looked at the documentation and read over the source and I can't seem to find a way to do this.

Please let me know if there is a solution, or if you would consider adding this feature, or perhaps I could take a stab at it and submit a PR.

Thanks!

It'd be great to have a method to reinitialize the accordion.

For example, I'm using it on an "About" page, with a collapsible list of staff members, and floated images in some body copy... not sure if it's because of the floats or because the accordion scripts are finishing before the js that's loading in the images, but the calculated maxHeight that's being tacked onto the panel isn't accounting for those images, so they're being cut off at the bottom if the body copy isn't long enough.

It would be an easy fix to be able to just call a reinitialize method on the panel after the image loads in.

Might not be an easy implementation though haha, I don't really know. Just an idea.

Love the plugin as a whole though! Thanks!

Maybe babel could be updated to the latest package @babel/preset-env as well as the object assign, since on newer babel configs the build will fail unless you install babel-preset-env and babel-plugin-transform-object-assign

Even the already compiled dist version asks for it 🤷‍♂️ so maybe is not that compiled already 🤔

Feature request

I use actual Version of badger-accordion: 1.2.4

Hi Stuart!
For deploying TypeScript without errors i need d.ts Version of badger-accordion.

Thanks

Expected Behavior

When accordion panels are collapsed, they should be hidden to all users.

Current Behavior & failure information

Presently when accordion panels are collapsed, screen reader users can access all of the contents of the collapsed accordions. Additionally, if there are focusable elements within an accordion panel, a sighted keyboard user navigating by the tab key would lose their focus on these elements.

Steps to Reproduce

1. Turn on a screen reader
2. begin navigating the page with the virtual cursor
3. content that should be hidden is announced to the screen reader

or

1. Add a focusable element/s to an accordion panel
2. Navigate the page with the tab key
3. focus will become "lost" when navigating from one accordion trigger to the next, as the hidden element(s) have received keyboard focus.

Fix

It's a rather simple fix to correct this. Add visibility: hidden to the CSS for the -ba-is-hidden class and when this class is on the containing element, the contents inside will be properly hidden to all users.

:((((((((((((((((