mapsteps / page-builder-framework

52 stars 17 forks source link

more hooks please #6

Closed gbruzzo closed 5 years ago

gbruzzo commented 5 years ago

Hello there, thank you for the last update, and for Custom Sections.

Could you please consider introducing 4 more hooks (2 pairs of open/close)

1st pair of hooks - on every page / single post template

-(Call it maybe 'NEW_HOOK1_OPEN') after
<div class="wpbf-grid wpbf-main-grid wpbf-grid-<?php echo esc_attr( $grid_gap ); ?>"> but before <?php do_action( 'wpbf_sidebar_left' ); ?> (when a the template includes a sidebar)

followed by (Call this maybe 'NEW_HOOK1CLOSE') after <?php do_action( 'wpbf_sidebar_right' ); ?> but before
</div> (this is the div that closes the grid div above)

and

2nd pair of hooks - on every page / single post template

-(Call it maybe 'NEW_HOOK2_OPEN') after <main id="main" class="wpbf-main wpbf-medium-2-3<?php echo wpbf_singular_class(); ?>"> before <?php wpbf_title(); ?>

followed by (Call it maybe 'NEW_HOOK2_CLOSE') after <?php do_action( 'wpbf_after_comments' ); ?> before </main>

You could place these also in the woocommerce-functions,.php wrappers - the functions corresponding to add_action( 'woocommerce_before_main_content', 'wpbf_woo_output_content_wrapper', 10 ); add_action( 'woocommerce_after_main_content', 'wpbf_woo_output_content_wrapper_end', 10 );

This way one could use these hooks to widgetize the Woocommerce templates as well.

I am asking this because

For the sake of illustration, here are the locations, when viewed on page-sidebar.php

<?php
/**
 * Template Name: Sidebar
 *
 * Page Template to display Content with Sidebar
 *
 * @package Page Builder Framework
 */

// exit if accessed directly
if ( ! defined( 'ABSPATH' ) ) exit;

$grid_gap = get_theme_mod( 'sidebar_gap', 'medium' );

get_header(); ?>

        <div id="content">

            <?php do_action( 'wpbf_content_open' ); ?>

            <?php wpbf_inner_content(); ?>

                <?php do_action( 'wpbf_inner_content_open' ); ?>

                <div class="wpbf-grid wpbf-main-grid wpbf-grid-<?php echo esc_attr( $grid_gap ); ?>">

                   <?php do_action('NEW_HOOK1_OPEN'); ?>

                    <?php do_action( 'wpbf_sidebar_left' ); ?>

                    <main id="main" class="wpbf-main wpbf-medium-2-3<?php echo wpbf_singular_class(); ?>">

                      <?php do_action('NEW_HOOK2_OPEN'); ?>

                        <?php wpbf_title(); ?>

                        <?php if( have_posts() ) : while ( have_posts() ) : the_post(); ?>

                        <div class="entry-content" itemprop="text">

                            <?php the_content(); ?>

                            <?php
                            wp_link_pages( array(
                                'before' => '<div class="page-links">' . __( 'Pages:', 'page-builder-framework' ),
                                'after'  => '</div>',
                            ) );
                            ?>

                        </div>

                        <?php endwhile; endif; ?>

                        <?php do_action( 'wpbf_before_comments' ); ?>

                        <?php comments_template(); ?>

                        <?php do_action( 'wpbf_after_comments' ); ?>

                      <?php do_action('NEW_HOOK2_CLOSE'); ?>

                    </main>

                    <?php do_action( 'wpbf_sidebar_right' ); ?>

                                       <?php do_action('NEW_HOOK1_CLOSE'); ?>

                </div>

                <?php do_action( 'wpbf_inner_content_close' ); ?>

            <?php wpbf_inner_content_close(); ?>

            <?php do_action( 'wpbf_content_close' ); ?>

        </div>

<?php get_footer(); ?>

You could also add the same 4 hooks to the Woocommerce wrappers in themes/page-builder-framework/inc/integration/woocommerce as follows

/* Wrappers */

// Add custom wrappers
add_action( 'woocommerce_before_main_content', 'wpbf_woo_output_content_wrapper', 10 );
add_action( 'woocommerce_after_main_content', 'wpbf_woo_output_content_wrapper_end', 10 );

// Start
function wpbf_woo_output_content_wrapper() {

    // vars
    $single_sidebar_position_global = get_theme_mod( 'woocommerce_single_sidebar_layout' );
    $sidebar_position_global = get_theme_mod( 'woocommerce_sidebar_layout' );
    $grid_gap = get_theme_mod( 'sidebar_gap', 'medium' );

    echo '<div id="content">';

    if ( is_product() ) {

        $id = get_the_ID();
        $single_sidebar_position = get_post_meta( $id, 'wpbf_sidebar_position', true );

        wpbf_inner_content();

        if( $single_sidebar_position && $single_sidebar_position !== 'global' ) {

            echo $single_sidebar_position !== 'none' ? '<div class="wpbf-grid wpbf-grid-'. esc_attr( $grid_gap ) .'">' : ''; 

                         do_action('NEW_HOOK1_OPEN'); 

            $single_sidebar_position == 'left' ? get_sidebar() : '';

            echo $single_sidebar_position !== 'none' ? '<main id="main" class="wpbf-main wpbf-medium-2-3'. wpbf_singular_class() .'">' : '<main id="main" class="wpbf-main'. wpbf_singular_class() .'">';

               do_action('NEW_HOOK2_OPEN'); 

        } elseif( $single_sidebar_position_global && $single_sidebar_position_global !== 'none' ) {

            echo '<div class="wpbf-grid wpbf-grid-'. esc_attr( $grid_gap ) .'">';

                            do_action('NEW_HOOK1_OPEN');

            $single_sidebar_position_global == 'left' ? get_sidebar() : '';

            echo '<main id="main" class="wpbf-main wpbf-medium-2-3'. wpbf_singular_class() .'">';

                         do_action('NEW_HOOK2_OPEN');

        } else {

            echo '<main id="main" class="wpbf-main'. wpbf_singular_class() .'">';

                        do_action('NEW_HOOK2_OPEN'); 

        }

    } else {

        echo '<div id="inner-content" class="wpbf-container wpbf-container-center wpbf-padding-medium">';

        if ( $sidebar_position_global && $sidebar_position_global !== 'none' ) {

            echo '<div class="wpbf-grid wpbf-grid-'. esc_attr( $grid_gap ) .'">';

                        do_action('NEW_HOOK1_OPEN');

            $sidebar_position_global == 'left' ? get_sidebar() : '';

            echo '<main id="main" class="wpbf-main wpbf-medium-2-3'. wpbf_archive_class() .'">';

                        do_action('NEW_HOOK2_OPEN');

        } else {

            echo '<main id="main" class="wpbf-main'. wpbf_archive_class() .'">';

                        do_action('NEW_HOOK2_OPEN');

        }
    }

}

// End
function wpbf_woo_output_content_wrapper_end() {

    $single_sidebar_position_global = get_theme_mod( 'woocommerce_single_sidebar_layout' );
    $sidebar_position_global = get_theme_mod( 'woocommerce_sidebar_layout' );

    if ( is_product() ) {

        $id = get_the_ID();
        $single_sidebar_position = get_post_meta( $id, 'wpbf_sidebar_position', true );

        if( $single_sidebar_position && $single_sidebar_position !== 'global' ) {

                   do_action('NEW_HOOK2_CLOSE');

            // main
            echo '</main>';

            // right sidebar
            $single_sidebar_position == 'right' ? get_sidebar() : '';

                         do_action('NEW_HOOK1_CLOSE');

            // grid
            echo $single_sidebar_position !== 'none' ? '</div>' : '';

            wpbf_inner_content_close();

        } elseif( $single_sidebar_position_global && $single_sidebar_position_global !== 'none' ) {

                       do_action('NEW_HOOK2_CLOSE');

            // main
            echo '</main>';

            // right sidebar
            $single_sidebar_position_global == 'right' ? get_sidebar() : '';

                        do_action('NEW_HOOK1_CLOSE');

            // grid
            echo '</div>';

        } else {

                         do_action('NEW_HOOK2_CLOSE');

            // main
            echo '</main>';

        }

    } else {

        if( $sidebar_position_global && $sidebar_position_global !== 'none' ) {

                         do_action('NEW_HOOK2_CLOSE');

            // main
            echo '</main>';

            // right sidebar
            $sidebar_position_global == 'right' ? get_sidebar() : '';

                        do_action('NEW_HOOK1_CLOSE');

            // grid
            echo '</div>';

        } else {

                          do_action('NEW_HOOK2_CLOSE');

            // main
            echo '</main>';

        }

        // inner content
        echo '</div>';

    }

    // content
    echo '</div>';

}

Please advise

Giacomo Bruzzo

mapsteps commented 5 years ago

Hi Giacomo,

thanks for the details!

I totally agree with you and more hooks are already on the roadmap. I've planned to add more while working on the Blog Layouts feature which is planned to be shipped with Page Builder Framework 2.0 next year.

I'm trying to get the hooks in before 2.0.

Best, David

gbruzzo commented 5 years ago

That is fantastic news, thank you

gbruzzo commented 5 years ago

Dear Mr Vongries

hope my email finds you well. I am sending you a short update on the work that followed from https://github.com/MapSteps/Page-Builder-Framework/issues/9#issuecomment-447660385

Hoping I am not going to steal time from your work, I just wanted to contextualise the points made in the issue above (really not an issue, but an enhancement request).

The point made there was that allowing for the function wpbf_search_menu_item to be overridable or pluggable is extremely useful when integrating other search plugins.

have a look at https://www.newrarenoise2.rarenoise.com : by overriding the function (I made it pluggable in theme-mods.php), I was able to remove most of the code you have surrounding your search fields that I did not want or need to be triggered (you also have javascript acting on it from site.js for example, and I did not want any of that) replacing it with relevant divs and links that can be used as a base for our own search plugin (one we had developed in the past) to hook into. I was not sure I was going to be able to do it, but thanks to the sheer quality and thinking that went into your framework, I was able to.

You may think - bad approach - why not just disable the search and add a widget or plugin via - you may be absolutely right, I am all ears and open to suggestions to improve my practices.

Let me know any thoughts you may have about the matter.

Kindest regards

Giacomo Bruzzo

On Thu, 6 Dec 2018 at 11:18, David Vongries notifications@github.com wrote:

Hi Giacomo,

thanks for the details!

I totally agree with you and more hooks are already on the roadmap. I've planned to add more while working on the Blog Layouts feature which is planned to be shipped with Page Builder Framework 2.0 next year.

I'm trying to get the hooks in before 2.0.

Best, David

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/MapSteps/Page-Builder-Framework/issues/6#issuecomment-444838225, or mute the thread https://github.com/notifications/unsubscribe-auth/AQBR-kGXQShNVZz6FIwqSTm6Zj2xfsgyks5u2P0OgaJpZM4Yx4MU .

gbruzzo commented 5 years ago

Hello There!

Noticed you have added hooks before/after loop (but they only kick in on archives?)

Cheers

Giacomo Bruzzo

mapsteps commented 5 years ago

Hey Giacomo,

good find! :) I've added the missing hooks to the WooCommerce pages - 6aa94b0f27f24f569a814c39619609ba17b5f5b9

Though, I'm not sure if it makes sense to add the new hooks (wpbf_before_loop & wpbf_after_loop) to the WooCommerce pages.

Here's why.

Basically, and to be semantically correct, these hooks would need to go to line 290 - https://github.com/MapSteps/Page-Builder-Framework/blob/6aa94b0f27f24f569a814c39619609ba17b5f5b9/inc/integration/woocommerce/woocommerce-functions.php

That's where the actual loop starts. Someone would probably expect the hook to be in line 288.

I'd rather suggest to override the existing WooCommerce filters (woocommerce_product_loop_start & woocommerce_product_loop_end) to inject additional code.

I'm not sure if it would work adding a hook here, while we're inside the filter. At least it doesn't feel right.

gbruzzo commented 5 years ago

Thank you for that - just one comment:

in wpbf_woo_output_countent_wrapper_end()

should the first

do_action( 'wpbf_inner_content_close' ); wpbf_inner_content_close(); not be put outside the first else loop, as follows?


/**
 * Wrapper End
 */
function wpbf_woo_output_content_wrapper_end() {

    $single_sidebar_position_global = get_theme_mod( 'woocommerce_single_sidebar_layout' );
    $sidebar_position_global        = get_theme_mod( 'woocommerce_sidebar_layout' );

    if ( is_product() ) {

        $id                      = get_the_ID();
        $single_sidebar_position = get_post_meta( $id, 'wpbf_sidebar_position', true );

        if( $single_sidebar_position && $single_sidebar_position !== 'global' ) {

            // main
            echo '</main>';

            // right sidebar
            $single_sidebar_position == 'right' ? get_sidebar() : '';

            // grid
            echo $single_sidebar_position !== 'none' ? '</div>' : '';

        } elseif( $single_sidebar_position_global && $single_sidebar_position_global !== 'none' ) {

            // main
            echo '</main>';

            // right sidebar
            $single_sidebar_position_global == 'right' ? get_sidebar() : '';

            // grid
            echo '</div>';

        } else {

            // main
            echo '</main>';

        }

                        /***** here! ******/

            do_action( 'wpbf_inner_content_close' );
            wpbf_inner_content_close();

    } else {

        if( $sidebar_position_global && $sidebar_position_global !== 'none' ) {

            // main
            echo '</main>';

            // right sidebar
            $sidebar_position_global == 'right' ? get_sidebar() : '';

            // grid
            echo '</div>';

        } else {

            // main
            echo '</main>';

        }

        do_action( 'wpbf_inner_content_close' );

        // inner content
        wpbf_inner_content_close();

    }

    do_action( 'wpbf_content_close' );

    // content
    echo '</div>';

Cheers

Giacomo

mapsteps commented 5 years ago

I've just discovered a bug in the WooCommerce sidebar settings so I think this is a good time to actually refactor this part. That's something I wanted to do for quite some time as this is a bit clunky compared to how we handle sidebars for the rest of the theme.

mapsteps commented 5 years ago

WooCommerce Sidebar refactor is done – 41615188945b0fcd2fd349410936be15cc31f63c

gbruzzo commented 5 years ago

You are a machine

gbruzzo commented 5 years ago

Hi there,

thanks for the update!

  1. testing the sidebar refactor, all works like clockwork at the moment - will update

  2. the two new hooks - before and after loop:

they would be useful for us there, in particular to inject content just above main content of a post or an archive, bounded by the sidebars, if present. We currently use a main wordpress hook (before content / after content), but would prefer to abandon that and only rely on your hooks for consistency.

Giacomo Bruzzo

gbruzzo commented 5 years ago

Hello there,

sorry to bump this topic again

the two new hooks - before and after loop:

I cannot find them on the page-sidebar, page, they cannot be chosen as hooks in custom sections - they would be useful for us there, in particular to inject content just above main content of a post or an archive, bounded by the sidebars, if present. We currently use a main wordpress hook (before content / after content), but would prefer to abandon that and only rely on your hooks for consistency.

To clarify, using the before_loop hook as an example, I see they have been added to

as

<?php if( have_posts() ) : ?>
<?php do_action( 'wpbf_before_loop' ); ?>
<?php while ( have_posts() ) : the_post(); ?>

and

<?php if( have_posts() ) : ?>
<?php wpbf_archive_header(); ?>
<?php do_action( 'wpbf_before_loop' ); ?>
<?php while ( have_posts() ) : the_post(); ?>

Would you consider adding them to other templates as page-sidebar.php or page.php

as

<?php wpbf_title(); ?>
<?php do_action( 'wpbf_before_loop' ); ?>
<?php if( have_posts() ) : while ( have_posts() ) : the_post(); ?>

and making these hooks accessible in custom sections?

Giacomo Bruzzo

mapsteps commented 5 years ago

Hey @gbruzzo,

I'm sorry, I thought I had answered your previous message but obviously, I haven't.

The 2 new hooks don't appear in custom sections as they're ment to be for internal use only and to inject wrappers to the archive pages specifically.

If you like, you can create a Pull Request with all the hooks added to all available archive/page templates. I think that would help taking this forward.

Best, David

gbruzzo commented 5 years ago

Hi there

understood, will do so then

Kind regards

Giacomo Bruzzo

mapsteps commented 5 years ago

Done. Finally :) 6fd4b90b018a7346411c0096120b853f021e933e