Bootstrap3 theme's phtml structure and blocks.

[FrancescoBrunoDev]

Hello everyone,

I've noticed that sometimes blocks like the following are used:

<nav class="breadcrumbs" aria-label="<?=$this->transEscAttr('Breadcrumbs') ?>">
  <div class="container">
    <?php if((!isset($this->layout()->showBreadcrumbs) || $this->layout()->showBreadcrumbs == true)
      && !empty($this->layout()->breadcrumbs)
      && $this->layout()->breadcrumbs !== false
    ): ?>
      <ul class="breadcrumb hidden-print"><?=$this->layout()->breadcrumbs ?></ul>
    <?php endif; ?>
  </div>
</nav>

But it would be more logical to have:

<?php if((!isset($this->layout()->showBreadcrumbs) || $this->layout()->showBreadcrumbs == true)
      && $this->layout()->breadcrumbs !== false
    ): ?>
    <nav class="breadcrumbs" aria-label="<?=$this->transEscAttr('Breadcrumbs') ?>">
      <div class="container">
        <?php if(!empty($this->layout()->breadcrumbs)): ?>
          <ul class="breadcrumb hidden-print"><?=$this->layout()->breadcrumbs ?></ul>
        <?php endif; ?>
      </div>
    </nav>
<?php endif; ?>

<?php if((!isset($this->layout()->showBreadcrumbs) || $this->layout()->showBreadcrumbs == true)
      && !empty($this->layout()->breadcrumbs)
      && $this->layout()->breadcrumbs !== false
    ): ?>
    <nav class="breadcrumbs" aria-label="<?=$this->transEscAttr('Breadcrumbs') ?>">
      <div class="container">
          <ul class="breadcrumb hidden-print"><?=$this->layout()->breadcrumbs ?></ul>
      </div>
    </nav>
<?php endif; ?>

(The first alternative would have the advantage that you could still have an empty space indicating the breadcrumbs are empty, while the second one would not.)

In fact, the second proposal avoids having an unnecessary empty <nav class="breadcrumbs"/> in case the breadcrumbs are disabled.

Or am I missing something? Thank you!

[demiankatz]

Thanks for bringing this up, @FrancescoBrunoDev.

The only reason I could think of to have the empty nav elements, etc., would be to allow styling a layout where the breadcrumb area of the page remains visibly consistent regardless of whether it is used on the current page. Whether that is a good idea, or whether we actually use that in any current theme designs, are separate questions -- but I'm just saying it's the only reason I could think of why we might potentially want those elements.

Even if we DO want to do that, I think your first proposal is an improvement that still supports that.

I would argue that in every scenario, we could further improve things by simplifying if((!isset($this->layout()->showBreadcrumbs) || $this->layout()->showBreadcrumbs == true) down to if ($this->layout()->showBreadcrumbs ?? true). I'm sure that original code was written before null coalescing was introduced to the language.

Please feel free to open a pull request if you'd like to try to move this forward further. I'd welcome input from others (especially @crhallberg and @EreMaijala) who might have further insights or opinions.

[crhallberg]

Today I'd be more partial to the second alternative and not having an empty element on the page. I could see the first alternative making more sense if we added a label to hold and define the space so that the element is never empty.

[FrancescoBrunoDev]

Thank you both for the feedback! I would like to contribute with a pull request with the second proposal, then. I will take care of it soon.

[demiankatz]

Sounds good! Thanks, @FrancescoBrunoDev!