Skip to:
Content

BuddyPress.org

Opened 13 years ago

Closed 13 years ago

Last modified 8 years ago

#3484 closed enhancement (fixed)

Inconsistency in action hook position in default theme files

Reported by: jonnyauk's profile Jonnyauk Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: 1.5 Priority: normal
Severity: normal Version:
Component: Templates Keywords: 2nd-opinion
Cc:

Description

I've been working through the latest trunk release (v1.5-beta2) default theme files and have noticed that in a number of files the 'before' and 'after' area specific action hooks change position in relation to the opening and closing 'content' and 'padder' divs.

I was wondering if there was a reason for this - or maybe just an oversight? I'll be happy to patch all the files, but don't want to trawl through them all if there is a reason for this!

In my humble opinion it would make much more sense to follow the code pattern in example 1 - where the specific action hooks are inside the 'content' and 'padder' divs. It would allow anyone adding content into this hook to automatically be 'inside' the content and padder divs. There are other hooks available if a developer wished to be 'outside' these main containing divs.

Another side-benefit is it makes the theme code a-lot more portable for theme and framework developers to surround the BP specific code in their own layout definitions. It provides a neat 'chunk' of code that can be put inside relevant layout code, rather than having to mess around with all the area specific action hooks.

I have attached a patch example for bp-themes/bp-default/members/index.php

For instance:

Example 1

file: bp-themes/registration/activate.php
'before' hook inside 'content' and 'padder' div

1<?php get_header( 'buddypress' ); ?>
2 
3	<div id="content">
4		<div class="padder">
5
6		<?php do_action( 'bp_before_activation_page' ) ?>

...

47		<?php do_action( 'bp_after_activation_page' ) ?>
48
49		</div><!-- .padder -->
50	</div><!-- #content -->

Example 2

file: bp-themes/members/index.php
'before' hook outside 'content' and 'padder' div

12<?php get_header( 'buddypress' ); ?>
13
14	<?php do_action( 'bp_before_directory_members_content' ); ?>
15
16	<div id="content">
17		<div class="padder">

...

81		</div><!-- .padder -->
82	</div><!-- #content -->
83
84	<?php do_action( 'bp_after_directory_members_content' ); ?>

Attachments (1)

patch.diff (998 bytes) - added by Jonnyauk 13 years ago.
Example optimised action hook structure for bp-themes/bp-default/members/index.php

Download all attachments as: .zip

Change History (11)

@Jonnyauk
13 years ago

Example optimised action hook structure for bp-themes/bp-default/members/index.php

#1 @boonebgorges
13 years ago

I agree that they should be consistent, and I agree that inside the padder makes the most sense. However, I'm a little wart of moving hooks at this point in the dev cycle. Are these inconsistencies present in BP 1.2.9 as well? We should grep through the plugin repo to see if publicly available plugins are using the hooks, or in some other way get a sense of how many existing themes will be messed up by this?

#2 @Jonnyauk
13 years ago

Agreed - it is important to check to see what the knock-on effects of this are for plugins and themes.

I've just had a look BP1.2.9 bp-themes/bp-default/members/index.php (as shown in example 2 above) and see that it has the following structure with the 'before' hook inside the 'content' and 'padder' divs.

BP1.2.9 bp-themes/bp-default/members/index.php

3 <div id="content">
4   <div class="padder">
5
6     <form action="" method="post" id="members-directory-form" class="dir-form">
7
8     <h3><?php _e( 'Members Directory', 'buddypress' ) ?></h3>
9
10    <?php do_action( 'bp_before_directory_members_content' ) ?>

...

51   <?php do_action( 'bp_after_directory_members_content' ) ?>
52
53   </form><!-- #members-directory-form -->
54
55   </div><!-- .padder -->
56 </div><!-- #content -->

The thing to note here is that the hook is also after the <h3> header (it is before this in BP1.5-beta2). I have no beef with the header, in-fact logic would suggest that this is probably correct and the action hook should come after the header right before the actual content starts?

Without checking deeper, it looks like there's a gremlin crept in there in BP1.5-beta2 default theme files (I checked out the trunk version before posting ticket). I've only compared these two files - but it certainly looks like the relevant 'before' and 'after' action hooks (bp_before_directory_members_content in this example) were originally inside the main 'content' and 'padder' divs in BP1.2.9

I'll be happy to offer to go through the default theme files and create patches for them if it is decided that this is appropriate action, it's the least I can do!

#3 @boonebgorges
13 years ago

Strange. It looks like the hook was moved in https://buddypress.trac.wordpress.org/changeset/3810

Would you mind looking through the other relevant template files to compare the current trunk position against 1.2.9? If the hooks were mostly after the header tag in 1.2.9, then we should use that as the standard across templates. If not, we'll have to make some decisions about how much stuff we're willing to break?

#4 @Jonnyauk
13 years ago

  • Owner set to Jonnyauk
  • Status changed from new to accepted

Absolutely - happy to help! I'll go over the trunk BP1.5-beta2 files and do a comparison against BP1.2.9. The structure should follow BP1.2.9 - and if they are all corrected there should be no problem with plugins and themes, easing migration to BP1.5.

Having the hook after the title makes sense to me - and if that's where it is in BP1.2.9 that's where it should stay;)

#5 @DJPaul
13 years ago

If we end up wanting to move an action that was introduced in the changeset that Boone links to, or any other new 1.5 action, I think that would be ok as they are obviously new anyway.

#6 @johnjamesjacoby
13 years ago

  • Milestone changed from Awaiting Review to 1.5
  • Owner changed from Jonnyauk to johnjamesjacoby
  • Status changed from accepted to assigned

#7 follow-up: @johnjamesjacoby
13 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [4994]) Normalize index actions in bp-default. Fixes regression from 1.2.9. Fixes #3484.

#8 @Jonnyauk
13 years ago

Looks like the decision has been made to have them outside the container divs.

I do understand why - it may be useful having a hook outside of these containing divs (in the default theme) - and more hooks are always welcome;)

As long as the approach/position is consistent I am happy!

#9 in reply to: ↑ 7 @Jonnyauk
13 years ago

Replying to johnjamesjacoby:

(In [4994]) Normalize index actions in bp-default. Fixes regression from 1.2.9. Fixes #3484.

Hi John - thanks for working through those files - I have noticed in quite a few other places throughout the core theme files that they need some more 'normalization', a number of inconsistent code patterns still exist (ie activity/index.php). Noticed you have been adding ';' - I personally prefer this too (and is consistent with the rest of the BP core).

#10 @DJPaul
8 years ago

  • Component changed from Appearance - Template Parts to Templates
Note: See TracTickets for help on using tickets.