cuny-academic-commons / cbox-theme

Default theme for Commons In A Box
GNU General Public License v2.0
20 stars 15 forks source link

add option for including groupblog posts in Recent Networkwide Blog Posts widget, fixes #226 #227

Closed JonathanReeve closed 9 years ago

JonathanReeve commented 9 years ago

This adds a checkbox to the widget's admin area which reads "Include groupblog posts." Checking this option will include groupblog posts in the widget display.

Screenshot:

selection_464

christianwach commented 9 years ago

@JonathanReeve I was under the impression that https://github.com/boonebgorges/bp-groupblog/commit/6dcffe8c68487490a2c8b1ca135cee0abf985d20 fixed this. Do you have the latest BuddyPress Groupblog plugin installed?

christianwach commented 9 years ago

@JonathanReeve Hmm, actually I may have responded too hastily. Looking again, it seems the intention of that commit was to merge groupblog posts with blog posts when blog posts are selected in the activity filter dropdown.

JonathanReeve commented 9 years ago

@christianwach, indeed, that commit fixes a different issue. I had a fix for that one in mlaa/cbox-mla@798cc4d, but thanks for telling me about Boone's fix--I'm glad that it's unnecessary now with the latest bp-groupblog, since my solution was quite a bit of a hack.

christianwach commented 9 years ago

@JonathanReeve It looks to me like you have caught an inconsistency in the CBOX theme here.

However, I've used group blogs as "workshop" spaces in the past (where there was definitely a distinction between the two types of activity type) and as a result I've argued elsewhere that the two types ought not to be merged by default. Perhaps an ideal solution would be for the two activity types to be separable or merged depending on the use case? A filter might do the trick.

I'm in favour of this PR being merged to fit the default behaviour of BuddyPress Groupblog, but will wait for others to chime in.

mkgold commented 9 years ago

Hi All -- FYI, Boone is on leave for the next month and likely won't have time to take a look at this until he's back.

JonathanReeve commented 9 years ago

@christianwach That makes sense. If I have time, I'll build a checkbox to the widget that can make it optional to include group blogs, so that group blogs can be filtered out like you describe, and I'll add it to this PR if it's still unmerged.

JonathanReeve commented 9 years ago

^ Added a checkbox to the widget admin, which reads "Include groupblog posts." Checking this will include groupblog posts in "Recent Networkwide Blog Posts"; unchecking it will only show non-groupblog posts. I'll update the PR title to reflect this.

christianwach commented 9 years ago

@JonathanReeve This looks great to me - thanks! @r-a-y Can we merge?

r-a-y commented 9 years ago

Thanks for the pull request, @JonathanReeve.

Instead of having separate variables for $query_string_a and $query_string_b, can we use just one?

eg.

$querystring = empty( $instance['include_groupblog'] ) ? 'action=new_blog_post' : 'action=new_blog_post,new_groupblog_post';
$querystring .= '&max=' . $real_max . '&per_page=' . $real_max;

Also, would it make sense to only show the "Include groupblog posts" checkbox if the BP Groupblog plugin is active? Open to feedback on this one.

Otherwise, looks solid!

JonathanReeve commented 9 years ago

@r-a-y, thanks for the suggestions. I'll make all those changes and add them to this PR.

I might be doing something wrong, but I can't quite seem to check for whether the BP Groupblog plugin is active using if ( is_plugin_active( 'bp-groupblog/bp-groupblog.php' ) ):. It works with if ( is_plugin_active( 'akismet/akismet.php' ) ), and I checked that bp-groupblog is active, so that leads me to suspect that maybe CBOX is blocking is_plugin_active in some way. Do you know of another way to check whether a CBOX plugin is active?

christianwach commented 9 years ago

@JonathanReeve I use if ( is_multisite() AND AND bp_is_active( 'groups' ) AND defined( 'BP_GROUPBLOG_IS_INSTALLED' ) )

JonathanReeve commented 9 years ago

Thanks @christianwach! I was going to use class_exists, but yours is way better. Added changes in 5052863 above.

r-a-y commented 9 years ago

Thanks again, @JonathanReeve! PR has been merged.