Hello,

Currently, The

wrapper for accordion content is getting added from JS. Check the below code for reference. 
            if (!this.usegroupheader) {
              $(this).siblings().wrapAll('<div></div>');
              row_count++;
            }

This works fine out of the box. However it creates issues in some other use cases mentioned below:

Issues:
- In case of multiple grouping of fields, The h3 and div structure gets messed up.
- When extending the views_accordion plugin, The JS produces unwanted HTML structure. Hence making the extensibility of the views_accordion plugin difficult.

Possible Solution:
- Instead of adding this

element from JS, Add it in TWIG file views-accordion-view.html.twig.

This will help the extensibility easy.

CommentFileSizeAuthor
#7 Screenshot from 2017-08-11 12-05-59.png37.2 KBmanuel garcia
#3 add_div_element_from-2893476-3.patch899 byteskunalkursija

Comments

kunal.kursija created an issue. See original summary.

kunalkursija’s picture

kunalkursija commented
Issue summary: View changes
kunalkursija’s picture

kunalkursija commented
StatusFileSize
new add_div_element_from-2893476-3.patch899 bytes

Adding Patch.

kunalkursija’s picture

kunalkursija commented
Assigned: kunalkursija » Unassigned
Status: Active » Needs review
manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

Thanks @kunal.kursija for this.

I am opened to this idea, and it is true that multiple grouping is a unsolved problem right now.

Quick review of the patch:
The JS only adds the wrapping div if this.usegroupheader is false (if that setting is off), so we'll have to do the same on the twig side, or using the group header as the accordion header will break.

About extending the plugin, I never really thought about it other than to override specific things, so I can't see how this gets affected by what you describe (I'd appreciate an example).

kunalkursija’s picture

kunalkursija commented

Hi @Manuel Garcia,

Yes, I noticed the todo comments w.r.t multiple grouping inside the plugin file.

Agreed with the conditional adding of wrapping div. However, i noticed that this.usegrouping is always true for both single and multiple grouping.
Reference- Check below code in public function preRender($result)

    $view_settings['usegroupheader'] = FALSE;
    foreach ($this->options['grouping'] as $group) {
      $view_settings['usegroupheader'] = $group['use-grouping-header'] == 1;
      // @TODO handle multiple grouping.
      break;
    }

The above piece of code makes the 'usegroupheader' as always TRUE, because of which the div tags from JS gets added every time.

Yes, I came across this issue when porting the views_nested_Accordion module to D8. A little bit about that module:

  • It's a module that provides one level of nesting/grouping.
  • To acheive its purpose, This module extends the viewsaccordion plugin and add's some configuration.
  • For rest of functionality it has it's own JS which sort of adds classes & does accordion things using jQuery functions.

So during my porting exercise, I noticed that the 'div' getting added from JS was causing an issue.

Please let me know what do you think.

Also for reference: Here is the issue regarding the D8 port of views nested accordion: https://www.drupal.org/node/2893497

manuel garcia’s picture

manuel garcia commented
StatusFileSize
new Screenshot from 2017-08-11 12-05-59.png37.2 KB

Thanks for kindly explaining the issue @kunal.kursija

The above piece of code makes the 'usegroupheader' as always TRUE, because of which the div tags from JS gets added every time.

This is not correct, usegroupheader is only TRUE if there is at least 1 grouping configured, AND if the option Use the group header as the Accordion header is enabled. Otherwise its always FALSE, since the option use-grouping-header defaults to 0.

So we do need to take into consideration this setting when we are printing the div ;-)

manuel garcia’s picture

manuel garcia commented
Status: Needs work » Needs review

We now have some basic test coverage, so triggering the tests here.

manuel garcia’s picture

manuel garcia commented

Setting it back to needs work based on #5 & #7.

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work
manuel garcia’s picture

manuel garcia commented
Status: Needs work » Needs review

Added some test coverage for field grouping on #3035375: Add test coverage for field grouping, let's see how the patch tests with it.