I tried to use the module as is on a Drupal 8.6 site. I ran into several issues:

  • Referral to Javascript modules caused PHP notices.
  • The drupalSettings were not accessible in the javascript module because of the difference in the blockid in javascript and PHP (dashes versus underscores).
  • When using colorbox, the colorbox did not scale to the window width.

The included patch fixes these issues and has a small cleanup of the code.

CommentFileSizeAuthor
#3 cbb-refactoring.patch2.75 KBardkuijpers
#3 cbb-bugfix.patch1.99 KBardkuijpers
cbb.patch4.89 KBardkuijpers

Comments

ardkuijpers created an issue. See original summary.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Issue summary: View changes
Status: Active » Needs work

@ardkuijpers thanks for reporting. You forgot to mark issue as 'Needs review' ;-)

As the patch has lot's of changes including code style stuff it is hard to see what is done for what reason.

The drupalSettings were not accessible in the javascript module because of the difference in the blockid in javascript and PHP (dashes versus underscores).

I guess this is the main bug right? The other issues are worth 2 new issues I guess :-)

Can you provide a fix only version version. Adding too many unrelated changes are hard to digest. Hope that's OK for you :-)

ardkuijpers’s picture

ardkuijpers commented
Status: Needs work » Needs review
StatusFileSize
new cbb-bugfix.patch1.99 KB
new cbb-refactoring.patch2.75 KB

Next time, I will make my fixes incremental and submit seperate reports. For now, I hope you will accept my comments as a way to help you.

If have split up the complete changes in two patch files. The first file addressess the bug fixes. I've put comments in the patch file so you can see (after the @@ signs) which of the following three bugs is addressed by each section:

  1. Referral to Javascript modules caused PHP notices
  2. drupalSettings were not accessible due to different blockid
  3. When using colorbox, the colorbox did not scale to the window width.

The second patch file addresses the refactoring I did to address the TODO comment that was in the code. So applying that patch is not strictly necessary (but recommended ;) )

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Assigned: Unassigned » clemens.tolboom

TNX! Will check these next week.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Very late :-( review of cbb-bugfix.patch which does not apply.

  1. +++ b/modules/contrib/cbb/cbb.js
@@ -43,9 +42,8 @@ FIXES #3 When using colorbox, the colorbox did not scale to the window width.
-                        $.colorbox({html: $block.html()});
-                    }
-                    else {
+                        $.colorbox({ html: $block.html(), maxWidth: '90%' });
+                    } else {

    The comment suggest 'window width' so why 90% 

  2. +++ b/modules/contrib/cbb/cbb.libraries.yml
@@ -6,4 +6,6 @@ FIXES #1 Referral to Javascript modules caused PHP notices 
-    - core/jquery.ui.accordion
\ No newline at end of file
+    - core/jquery
+    - core/jquery.cookie
+    - core/drupalSettings
\ No newline at end of file

    Makes me wonder why I added jquery.ui.accordion then ? 

  3. +++ b/modules/contrib/cbb/cbb.module
@@ -58,20 +58,17 @@ FIXES #1 Referral to Javascript modules caused PHP notices, and #2 drupalSettings were not accessible due to different blockid
-	'block-' . $build['#id'] => array(
+    Html::cleanCssIdentifier('block-' . $build['#id']) => array(

    Looks great to do

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Needs review » Fixed

I've applied fixes from cbb-bugfix.patch apart from the colorbox 90%.

Patch cbb-refactoring.patch needs a reroll in a #3027336: Remove necessity to copy settings

Mark this as fixed as we have a followup.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Maybe the colorbox 90% needs a new issue too.

ardkuijpers’s picture

ardkuijpers commented

Sorry for the confusing comment. I noticed that the colorbox didn't scale at all so I added the maxWidth attribute. I used 90% because I like that better than to scale to the complete width of the window (which is achieved with maxWidth: 100%).

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.