Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pivica created an issue. See original summary.

pivica’s picture

First working patch.

Berdir’s picture

+++ b/themes/bs_bootstrap/bs_bootstrap.theme
@@ -174,6 +180,16 @@ function bs_bootstrap_form_system_theme_settings_alter(&$form, FormStateInterfac
+    '#options' => [
+      'on' => t('On'),
+      'off' => t('Off'),
+    ],
+    '#default_value' => bs_bootstrap_get_setting('navbar.onhover') ? 'on' : 'off',

can't you use 0 and 1 as keys so it will be converted to that automatically on save due to config schema (and if you don't have config schema yet than that should be changed)

pivica’s picture

Okey here is a new version based on feedback from comment 3.

But, no idea why it does not work properly. When I save setting for bs_bootstrap theme for example for onhover Drupal will save a string and not a boolean:

drush cget bs_bootstrap.settings bs_bootstrap.navbar
'bs_bootstrap.settings:bs_bootstrap.navbar':
  onhover: '0'

So it stored '0' string instead of boolean false?

pivica’s picture

Added form state for onhover select box so it will be displayed only when navbar_type is second level horizontal.

The problem from comment 4 still remains.

pivica’s picture

If hovering would be that simple... ;)

Here is a new version with much-improved hover handling with lot of UX improvements. Changes:

  • Fixed click bug when clicking on hovering item. No need to double click now, it works as expected.
  • Added support for jQuery.hoverIntent. Note that we are not loading it, code will check if hoverIntent exist and then it will use it instead of standard jQuery.hover.
  • Hovering is now done on li element and not an element. This allows us to keep hovering when pointer is transitioning from item to sub-items.
  • We will keep for now active class on parent item when hovering subitems, so user knows exactly which parent element subitems are belonging to.
  • Code refactoring.
Berdir’s picture

You're not going to like this. Also, sorry for not spotting this earlier, also had to debug it pretty deep to spot the fairly obvious mistake, you're missing a mapping: at the top.

pivica’s picture

Status: Needs review » Fixed

Damn such a stupid mistake from me...

Great you found it. Committed.

  • pivica committed d34e620 on 8.x-1.x
    Issue #2935920 by pivica, Berdir: Show sub-navigation on hover
    

Status: Fixed » Closed (fixed)

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