ContextInstitute / bfcom

http://bfn.context.org
GNU General Public License v3.0
4 stars 1 forks source link

Highlighting the More ... link – a job for JS? #97

Closed rgilman closed 5 years ago

rgilman commented 5 years ago

I'm stumped on how to appropriately highlight the More ... link in the submenu when it's dropdown contains the selected menu item. That is, I can't see how to do this via either php or pure CSS. I wonder if it could be done with JS.

We need some kind of test to determine if the selected menu item is visible in the dropdown. That's dependent on the viewport size. I'm imagining that if this tests true then we should add an "active" class to the More ... link. Maybe there are other ways that would be simpler.

Any ideas on how we might do this?

This is a residual issue from #91, which I will now close in favor of this issue.

iangilman commented 5 years ago

The way to do this with PHP + CSS would be to introduce some new classes that you can add at a higher level. For instance, right now if #members-groups-li is selected, we add the current and selected classes to it. We could introduce a new class like members-groups-selected that we could apply to .header-submenu and then with some complicated media queries we can determine whether the "more" button should be highlit (basically .header-submenu.members-groups-selected .menu-more for certain widths).

That said, I think it would be a lot cleaner in JavaScript. If we have jQuery (which it looks like we do, but we might want to upgrade it to a more recent version) we could do something like this:

$(window).on('resize', function() {
  var $more = $('.menu-more');
  $('#menu-subnav-1 .bp-groups-tab').each(function(el, i) {
    var $item = $(el);
    if (!$item.is(':visible') && $item.hasClass('selected')) {
      $more.addClass('selected');
    }
  });
});

Of course if we're doing that, it might be cleaner to also handle the menu hiding/showing in JavaScript as well.

Note that it looks like the jQuery on the site is named jQuery and not $. You can easily fix that with var $ = jQuery.

rgilman commented 5 years ago

I would be delighted to handle the menu hiding/showing in JavaScript. The current approach with lots of arbitrary breakpoints feels kludgy and fragile. How can we go about this? @iangilman , what role might you take with this?

As for the version of jQuery, it looks like what get's loaded by WP v5 is v1.12.4 . Foundation at this point comes with v3.3.1, which is the latest version. From the bit of research I've done, it looks like there are two ways to upgrade from what WP offers by default:

  1. The replacement way –
    
    // include custom jQuery - put in bfc-functions.php
    function bfc_include_custom_jquery() {
    wp_deregister_script('jquery');
    wp_enqueue_script('jquery', 'https://ajax.googleapis.com/ajax/libs/jquery/3.3.1/jquery.min.js', array(), null, true);
    }
    add_action('wp_enqueue_scripts', 'bfc_include_custom_jquery');
(From [Including jQuery in WordPress (The Right Way)](https://digwp.com/2009/06/including-jquery-in-wordpress-the-right-way/))

This updates jQuery for everything so it might break some plugins or WP core functions. On the other hand, since we control the space, we could likely deal with those issues.

2. The noConflict way –

// Again put in bfc-functions.php wp_register_script( 'jquery3.3.1', 'https://code.jquery.com/jquery-3.3.1.min.js' ); wp_add_inline_script( 'jquery3.3.1', 'var jQuery3_3_1 = $.noConflict(true);' ); wp_enqueue_script( 'plugin-javascript', plugins_url( 'js.js', FILE ), array( 'jquery3.3.1' ) );

(From [INCLUDING A DIFFERENT JQUERY VERSION IN WORDPRESS](https://wpengine.com/support/including-a-different-jquery-version-in-wordpress/))

Here the third line enqueues a particular JS file and names jquery3.3.1 as a dependency. Then, the functions in that file would have the form:

( function( $ ) { // Code goes here. }( jQuery3_2_1 ) );


This is cleaner but it also deprives other components (like Foundation) of the updated jQuery.

I'll also note that I found a WP plugin called [Version Control for jQuery](https://wordpress.org/plugins/version-control-for-jquery/) which keeps the site up-to-date with the latest version.

@iangilman, you're the expert here. What are your thoughts on all this?
rgilman commented 5 years ago

I've been doing more research and want to record some links here.

This thread (https://wordpress.stackexchange.com/questions/257317/update-jquery-version ) recommends the following variant for the replacement way -

function replace_core_jquery_version() {
    wp_deregister_script( 'jquery-core' );
    wp_register_script( 'jquery-core', "https://code.jquery.com/jquery-3.3.1.min.js", array(), '3.3.1' );
    wp_deregister_script( 'jquery-migrate' );
    wp_register_script( 'jquery-migrate', "https://code.jquery.com/jquery-migrate-3.0.1.min.js", array(), '3.0.1' );
}

More info on jQuery Migrate is at https://jquery.com/download/#jquery-migrate-plugin and https://github.com/jquery/jquery-migrate#migrate-older-jquery-code-to-jquery-30 .

It looks to me like we could test our current system using the uncompressed versions of jQuery Migrate and then move on to jQuery 3.3.1, keeping the compressed version of jQuery Migrate installed as insurance re: new plugins.

iangilman commented 5 years ago

I think we should aspire to having everything using the new jQuery. jQuery Migrate seems like reasonable insurance.

A possible JavaScript function for managing the hiding/showing of the "more" menu items could look something like this:

function updateMenu() {
  var $subnav1 = $('#menu-subnav-1');
  var $subnav2 = $('#menu-subnav-2');
  var $more = $subnav2.find('#more');
  var $items1 = $subnav1.find('li');
  var $items2 = $subnav2.find('.more-ul li');
  var targetWidth = window.innerWidth - 200;

  $more.hide();
  $items2.hide();

  var i;
  for (i = $items1.length - 1; i >= 0; i--) {
    if ($subnav1.width() > targetWidth) {
      $items1.eq(i).hide();
      $items2.eq(i).show();
      $more.show();
    }
  }
}

updateMenu();

$(window).on('resize', updateMenu);

I haven't played with it enough to know if that width test is enough; for instance if the overflow wraps onto a new line we would need to check the height rather than the width. Anyway, this is the basic idea.

rgilman commented 5 years ago

Sounds good. How should we proceed to test and implement this?

BTW, How to Properly Add jQuery Scripts to WordPress is a good reference.

iangilman commented 5 years ago

I'm happy to coach you through adding it in if you'd like. Otherwise perhaps @DavidScottBernstein can take a whack at it.

rgilman commented 5 years ago

@iangilman, I'd like to take you up on your offer, thank you. I feel it would be good for me to understand some basic patterning and for the two of us to blend your JS knowledge with my WP knowledge. I expect we will have future opportunities for @DavidScottBernstein to get the benefit of your JS coaching.

iangilman commented 5 years ago

Sounds good! So, add jQuery in per the instructions you found, and add my code from above into assets/scripts.js and see what happens! Of course you'll want to disable the media queries. The script as is probably isn't perfect, but it's a start. One thing I can think of is we may want to detect what kind of page we're on and only do it on the appropriate kinds of pages. On the other hand, I guess every page has a top nav, so maybe it's not necessary.

rgilman commented 5 years ago

I've followed your instructions and nothing seems to break. However, functionally it's not there yet. More ... flashes on and then disappears as I resize the screen. After hiding the last submenu item and moving it to the More dropdown, the rest of the submenu items just wrap around to a second line. Expanding the screen doesn't restore it to its initial state.

rgilman commented 5 years ago

Nevertheless, it looks promising and I'm confident we can get it working. What are your thoughts for a next step?

iangilman commented 5 years ago

How about you push your branch and I'll take a look?

rgilman commented 5 years ago

For the record, Ian and I worked on this together on Sunday with good success. The result has now been merged in the master via PR #100.