mapsteps / page-builder-framework

52 stars 17 forks source link

Questions about search (not quite an issue) #9

Closed gbruzzo closed 5 years ago

gbruzzo commented 5 years ago

Hello there!

we have a few questions about the search function

thank you in advance

Giacomo

gbruzzo commented 5 years ago

Hello there!

a few updates.

I can see that the switch in search engine happens in function wpbf_search_menu_itemfrom /inc/theme-mods.php .

This function is called both by wpbf_search_menu_icon and by wpbf_search_menu_icon_mobile

If I understand correctly, one could either make wpbf_search_menu_item filterable, replacing line 103 of inc/theme-mods.php by something like

$search_item = apply_filters( 'wpbf_search_menu_item_filter' , $search_item );
return $search_item;

(I am having some problems though with wpbf_search_menu_icon_mobile - when using this approach)

or make the whole function pluggable, wrapping it with

if ( ! function_exists( 'wpbf_search_menu_item' )) {
....
}

this works out of the box, but I am aware pluggable functions are not the preferred approach nowadays.

The reason for this exercise is not only to add granularity to the choice of search engine (by page etc), but also to allow for the template to be altered. This may help to integrate a different search plugin all together, though maybe one could simply use a hook and associate a widget position with that particular hook (which one? not completely clear yet)

This kind of answers the first two questions, but maybe there is a much easier way?

Would you consider adding a filter to wpbf_search_menu_item or make the function pluggable as an enhancement in any case?

Thanks in advance

Giacomo

gbruzzo commented 5 years ago

Hello there

I have managed to bypass the need to override / replace wpbf_search_menu_item.

by using the hooks you had provided (wp_nav_menu_items and wpbf_before_mobile_toggle)

add_action( 'wpbf_before_mobile_toggle', 'Custom_search_icon_mobile', 20 );

function Custom_search_icon_mobile ( $items ) {

    // vars
    $class = 'wpbf-mobile-nav-item';
    $items = '';

    // we have a slightly different markup for the search menu item if it's being displayed outside the main menu
    $items .= '<div class="'. $class .'"><a href="#" class="search-everything-trigger" data-search-action="open" data-uk-toggle="{target:'."'.wpbf-navigation'".'}">';
    $items .=  '<i class="wpbff wpbff-search" aria-hidden="true"></i>';
    $items .= '</a></div>';
    echo $items;
}

and

add_filter( 'wp_nav_menu_items', 'Custom_search_menu_icon', 20, 2 );
function Custom_search_menu_icon( $items, $args ) {
    // stop here, if we have an off-canvas menu
    if( wpbf_is_off_canvas_menu() ) return $items;

    // only add the Search Menu Item to the main navigation 
    if( $args->theme_location == 'main_menu' ) {
            // vars
            $class = 'wpbf-nav-item';
            // we have a slightly different markup for the search menu item if it's being displayed outside the main menu
            $items .= '<li class="menu-item"><a href="#" class="search-everything-trigger" data-search-action="open" data-uk-toggle="{target:'."'.wpbf-navigation'".'}">';
            $items .= '<i class="wpbff wpbff-search" aria-hidden="true"></i>';
            $items .= '</a></li>';
        }
    return $items;
}

no need to override wpbf_search_menu_item anymore.

FYI, the original function was

if ( ! function_exists( 'wpbf_search_menu_item' )) {
function wpbf_search_menu_item( $is_navigation = true, $is_mobile = false ) {
    // vars
    $class = $is_mobile ? 'wpbf-mobile-nav-item' : 'wpbf-nav-item';
    $search_item = '';

    // we have a slightly different markup for the search menu item if it's being displayed outside the main menu
    $search_item .= $is_navigation ? '<li class="menu-item"><a href="#" class="search-everything-trigger" data-search-action="open" data-uk-toggle="{target:'."'.wpbf-navigation'".'}">' : '<div class="'. $class .'"><a href="#" class="search-everything-trigger" data-search-action="open" data-uk-toggle="{target:'."'.wpbf-navigation'".'}">';
    $search_item .=  '<i class="wpbff wpbff-search" aria-hidden="true"></i>';

    $search_item .= $is_navigation ? '</a></li>' : '</a></div>';
    return $search_item;
    }
}

but that required a change in theme-mods.php as well.

PS : the data-* statements and are required for our own search plugin to work

Summarising: override not required to replace the search with a different one.

mapsteps commented 5 years ago

Hi Giacomo,

the built-in search functionality has been refactured just recently.

If WooCommerce is installed, we've decided to call get_product_search_form() instead of get_search_form. I'm open to suggestions here.

To create a custom search form, why not filter through get_search_form? Would that do the trick?

add_filter( 'get_search_form', 'prefix_custom_search_form' );
function prefix_custom_search_form( $search_form ) {
    $search_form = // your custom form goes here
    return $search_form;
}
mapsteps commented 5 years ago

In reply to – https://github.com/MapSteps/Page-Builder-Framework/issues/6#issuecomment-447866862 (Let's keep everything organized 😄)

As you've mentioned, we could make wpbf_search_menu_item function filterable. That will allow us to override the default markup entirely. Keep in mind that get_search_form can be filtered as well.

wpbf_search_menu_item is being called in multiple instances (main navigation, mobile header, off-canvas header), so if someone uses the filter to override the markup, it kind of needs to be very similar to what we already have to function correctly.

This can easily be overlooked by someone using the filter. Do you have an example on how you'd use the filter? I'd love to test this on my end to see if the search item is being displayed properly in all instances.

Alternatively, I'd keep the menu item switched off in the customizer and use the functions in the theme-mods.php file as an example to create a custom one.

Best, David

gbruzzo commented 5 years ago

Hello there!

thank you for your elaborate response.

I think I conflated two issues into one (as I often do - sorry), and my resulting reasoning was not very clear.

Issue 1: Allow user to decide whether what 'internal/standard' search engine to use (e.g. standard wordpress vs standard woocommerce). Either a switch or a hook / filter to allow users to override in functions .php

Bear in mind that some 3rd party search plugins (SearchWP, for instance) require get_search_form to be present (they take it over). This cannot happen, if get_product_search_form is being used.

In synthesis : an override more than a rigid switch would be desirable. This way one could decide to keep a product search (with multiple if statements in function.php) on the shop pages and have a wider search on custom posts and generic pages. Just a thought of course.

The override would have to be really similar to your own, and only change this part of the function,

// if we have a shop, we're going to call the product search form
    if ( class_exists( 'WooCommerce' ) ) {
        $search_item .= get_product_search_form( $echo = false );
    } else {
        $search_item .= get_search_form( $echo = false );
    }

to something like

    if ( class_exists( 'WooCommerce' ) && (is_shop() || is_product() || ...  || ...)) {
        $search_item .= get_product_search_form( $echo = false );
    } else {
        $search_item .= get_search_form( $echo = false );
    }

or just replacing it with

$search_item .= get_search_form( $echo = false );

and overriding the search form itself or letting a plugin take it over (e.g. SearchWP or similar ones.

You are absolutely right, though, the risk of messing it up is great. I was able to avoid the override completely and only use the two other hooks (see Issue 2).

Issue 2: I was actually trying to replace all the html with another setup entirely, one required by a plugin I had asked a third party to develop about a year ago. The solution is not to override wpbf_search_menu_item but to exploit the hooks wp_nav_menu_items and wpbf_before_mobile_toggle to inject the relevant code, all the while switching the search off in Customizer for both regular and mobile layouts.

(the code is described above in https://github.com/MapSteps/Page-Builder-Framework/issues/9#issuecomment-448220880)

www.newrarenoise2.rarenoise.com - you can see how it works

Many thanks again - this is a fantastic framework

mapsteps commented 5 years ago

Hi @gbruzzo,

sorry for the delay here, I was taking some time off between the years. Hope you started well in 2019! :)

I've added a toggle (turned off by default) so the menu item specifically can be turned into a product search by just a click of a button.

With that solution, we don't interfere with other plugins that filter get_search_form() like SearchWP for instance.

40b9ad57c26f7be8829998fb20dfc3c498e0bfa1

gbruzzo commented 5 years ago

Dear Mr Vongries,

Great solution, excellent, many thanks.

Have installed the 2.0 branch for testing: so far so very good.

Giacomo Bruzzo

On Fri, 4 Jan 2019 at 14:42, David Vongries notifications@github.com wrote:

Hi @gbruzzo https://github.com/gbruzzo,

sorry for the delay here, I was taking some time off between the years. Hope you started well in 2019! :)

I've added a toggle (turned off by default) so the menu item specifically can be turned into a product search by just a click of a button.

With that solution, we don't interfere with other plugins that filter get_search_form() like SearchWP for instance.

40b9ad5 https://github.com/MapSteps/Page-Builder-Framework/commit/40b9ad57c26f7be8829998fb20dfc3c498e0bfa1

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/MapSteps/Page-Builder-Framework/issues/9#issuecomment-451462263, or mute the thread https://github.com/notifications/unsubscribe-auth/AQBR-r7m0ARrHLSZd9NERn94lWyic1cqks5u_2hygaJpZM4ZOJ0y .