Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accordion expand/collapse option #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nmillin
Copy link

@nmillin nmillin commented Feb 6, 2017

adding the ability to have all panels open, all closed, or just the first open. Multiple panels open functionality added.

This is from https://h5p.org/comment/9900. I used language that made sense to me, but I don't have strong feelings about what the labels, terms, and descriptions are.

Please let me know what you think. Thanks.

-Nate

…irst open. Multiple panels open functionality added.
@tajakobsen
Copy link

Hi @nmillin ,

Code looks very good! But I found a bug:

  • I create two panels, and I choose "Only first panel expanded" and "One accordion open at a time" in the editor.
  • When I view the content, I can toggle the second panel, without the first panel closing (unless I have already manually closed the first panel).

Copy link

@tajakobsen tajakobsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but the bug where initially expanded panel is not collapsed even if "One accordion open at a time" is set.

…t a time" selected.

- Now it will set the $expandedTitle & $expandedPanel on this case.
@nmillin
Copy link
Author

nmillin commented Apr 21, 2017

Thanks for the review tajakobsen.
I confirmed the bug. It looks like $expandedTitle & $expandedPanel need to be set on self when the first panel is expanded by default. I'm a bit confused why that is tracked the way it is, but I'm probably missing something.

Please see 7436468 with a fix for this bug. Thanks.

Copy link

@tajakobsen tajakobsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace id based selector with reference to element.

h5p-accordion.js Outdated
// Since the first panel is already open, and this is the first time through, you need to make sure $expandedTitle & $expandedPanel are set.
if (loadFirstPanel && expandCollapseOption === "expandedFirstOnly") {
self.$expandedTitle = $('#h5p-panel-link-0-0'); // Get the first panel title.
self.$expandedPanel = $('#h5p-panel-content-0-0'); // Get the first panel.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this accordion is not the first accordion on the page? Then the -0-0-part of the selectors wouldn't work.

Maybe you can check if this works:

self.$expandedTitle = self.elements[0];
self.$expandedPanel = self.elements[1];

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming there are content types that could have 2 (or more) accordions as part of a larger content type. Is that correct?

A little tweak to the code so that it will return a jQuery object instead of html.
self.$expandedTitle = $(self.elements).eq(0);
self.$expandedPanel = $(self.elements).eq(1);

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a commit with this change.

@fnoks
Copy link
Contributor

fnoks commented Aug 16, 2017

This issue is tracked here: https://h5ptechnology.atlassian.net/browse/HFP-722

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants