Hello,

I am trying to use the bootstrap_carousel integration with for example the following renderable array in a preprocess field:

$variables['carousel'] = [
  '#theme' => 'bootstrap_carousel',
  '#controls' => TRUE,
  '#indicators' => TRUE,
  '#interval' => $interval,
  '#slides' => $slides,
];

The indicators are generated automatically in a preprocess inside the bootstrap theme. Which is great thanks for the automation.

The problem I have is that the "#target" is not passed to the bootstrap/templates/bootstrap/item-list--bootstrap-carousel-indicators.html.twig

So the data-target attributes is empty and the indicators does not work.

I think it is due to the item_list hook_theme that does not register a target variable.

I will try to make a patch (this week).

CommentFileSizeAuthor
#7 bootstrap-data_target_carousel-2878764-7.patch2.08 KBGrimreaper
#4 bootstrap-data_target_carousel-2878764-4.patch2.26 KBGrimreaper
#2 bootstrap-date_target_carousel-2878764-2.patch1.93 KBGrimreaper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper created an issue. See original summary.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper as a volunteer commented
Assigned: Grimreaper » Unassigned
Status: Active » Needs review
FileSize
bootstrap-date_target_carousel-2878764-2.patch1.93 KB

Hello,

Here is a patch that use the context variable, declared in the item_list theme, to pass the target id to the indicator item list.

Thanks for the review.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Needs work 
+++ b/templates/bootstrap/item-list--bootstrap-carousel-indicators.html.twig
@@ -22,7 +22,7 @@
+      <li{{ item.attributes.addClass(item_classes) }} data-target="{{ context.target }}" data-slide-to="{{ loop.index0 }}"></li>

Using context will be confusing since twig also has the global variable: _context.

We can use #context internally, but item_list__bootstrap_carousel_indicators should be preprocessed to convert it to a normal target variable from #context.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper as a volunteer commented
Status: Needs work » Needs review
FileSize
bootstrap-data_target_carousel-2878764-4.patch2.26 KB

Hello,

Thanks for your suggestion. I agree that a context variable can be misleading.

Here is a patch that take your suggestion into account.

Thanks for the review.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Needs work 
  1. +++ b/src/Plugin/Preprocess/ItemListBootstrapCarouselIndicators.php
@@ -0,0 +1,31 @@
+    // Add a target variable from the item list context data.
+    $context = $variables->offsetGet('context', []);
+
+    if (isset($context['target'])) {
+      $variables->target = $context['target'];
+    }

    All that's needed here is

    $variables->target = $variables->getContext('target');

    See Variables::getContext. 

  2. +++ b/src/Plugin/Preprocess/ItemListBootstrapCarouselIndicators.php
@@ -0,0 +1,31 @@
+    // Ensure all attributes are proper objects.
+    $this->preprocessAttributes();

    This shouldn't be needed.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented

Also, considering that this is a theme hook suggestion and in this base theme, I think it makes sense to call parent::preprocessVariables() before everything in case sub-themes add a preprocessor on item_list.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper as a volunteer commented
Status: Needs work » Needs review
FileSize
bootstrap-data_target_carousel-2878764-7.patch2.08 KB

Thanks for the review.

Here is an updated patch.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Fixed
markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented

A new issue should be created for #start_index that does the same thing actually.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper as a volunteer commented

Thanks for the commit :)

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper as a volunteer commented

Ok, I will see that during the week or this weekend.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper as a volunteer commented
Related issues: +#2880010: Some Bootstrap templates do not have variables

Adding follow up issue.

Status: Fixed » Closed (fixed)

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