Problem/Motivation

Current implementation of dropdown is very complicated:

  • split between dropdown & dropdown_item is useless
  • call to button pattern is hardcoded in Twig (3 times!)
    {% if variant|lower != 'btn_group__dropstart' %}
      {{ pattern('button', {
        label: title,
        attributes: split_button_attributes
      }, button_variant) }}
    {% endif %}

    {{ pattern('button', {
      label: 'Toggle Dropdown'|t,
      label_visually_hidden: true,
      attributes: button_attributes.addClass('dropdown-toggle-split').setAttribute('id', false)
    }, button_variant) }}
  {% else %}
    {{ pattern('button', {
      label: title,
      attributes: button_attributes
    }, button_variant) }}
  {% endif %}

Source: https://git.drupalcode.org/project/ui_suite_bootstrap/-/blob/5.0.x/templ...

  • the different cases (split, button group, direction...) are managed by 6 variants & 8 settings

Source: https://git.drupalcode.org/project/ui_suite_bootstrap/-/blob/5.0.x/templ...

Proposed resolution

On ui_suite_bootstrap 4, we have a simpler implementation:

Of course, BS5 dropdown is a bit more complicated than BS4 dropdown:

But we guess it will be OK to add them while keeping the simple structure.

Remaining tasks

I will try to propose a MR soon.

This MR will also change the call to dropdown from navbar-nav.

API changes

Yes, but still in alpha so it is OK.

Issue fork ui_suite_bootstrap-3338283

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: pdureau » grimreaper
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Active » Needs review

Grimreaper opened merge request !49

Grimreaper opened merge request !50

Grimreaper closed merge request !49

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

I opened the new MR https://git.drupalcode.org/project/ui_suite_bootstrap/-/merge_requests/50, because previous one needed a rebase and to make the rebase easier I squashed the commits so to not lose the history, I pushed this squashed and rebased version on a new branch, so new MR.

Grimreaper opened merge request !51

  • Grimreaper committed f266253a on 5.0.x authored by pdureau 
    Issue #3338283 by Grimreaper, pdureau: Add brand field in navbar. Add...
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » pdureau
Status: Needs review » Needs work

As discussed, MR 50 had not been tested on real usage with menus, and modules like https://www.drupal.org/project/menu_link_attributes to allow to add classes/attributes on the link and/or on the wrapper.

Also, the nav pattern had been forgotten in the refactoring.

I tried to make it work, but it is too time consuming and I have side effects on side effects.

The main problems:
- in real menu links, "url" is a URL object which options contains the attributes that will be used on the a tag.
- also the route of the URL object can determine extra behavior like that will display a span and that will display a button.
- in real menu links, attributes is then for the link wrapper.
- not using the link() method bypass attributes put on the link. And do not use the link generator system of Drupal that can be altered.
- so I don't think the patterns should be done based on a structure easily manipulable in YAML preview structure but based on a "real" structure.

I know there is the subject of removing PHP preprocess from themes, but I think here it is needed.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

The navbar rework had not taken into account:
- ui_suite_bootstrap/templates/overrides/block/block--system-branding-block.html.twig
- ui_suite_bootstrap/templates/overrides/system/page.html.twig
- adding a new region to the theme.

Also navbar-brand class should be added directly on the link not on a div around the link.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Currently navbar collapse does not work anymore on mobile.

I am trying to figure out why and when this happened.

pdureau’s picture

pdureau commented

The next opportunity to simplify this pattern will be by leevraging this new setting type: #3345071: Add links setting type

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Related issues: +#3345071: Add links setting type

I found the problem about the navbar on mobile #3346067: Page navbar action should be collapse.

I think I also found out (I need to check by settings back theme to Olivero or Bootstrap 3) that the attributes allowing to mark links as active are not properly transmitted.

So native active system does not work and it prevents modules like https://www.drupal.org/project/menu_trail_by_path to work.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

When updating #3292600: Bootstrap 5 : Components > Toasts MR, I found out that |default('true') or |default(true) was bad for ensuring boolean settings default value. Ok for false, but as false or null is evaluated the same way having |default(true) force the setting value to true even when FALSE.

The only pattern having that is the dropdown in: {% set auto_close = auto_close|default('true') %}

pdureau opened merge request !113

pdureau closed merge request !113

pdureau’s picture

pdureau commented
Title: Bootstrap 5: dropdown is too complicated » Bootstrap 5: dropdown is too complicated (outdated)
Assigned: pdureau » Unassigned
Status: Needs work » Closed (outdated)

We move here #3369769: Bootstrap 5: Convert menu, breadcrumb and pagers to the links prop type

Grimreaper closed merge request !50