Skip to:
Content

Opened 20 months ago

Closed 17 months ago

Last modified 17 months ago

#7079 closed enhancement (fixed)

BP-legacy: split group single admin.php in separate template files

Reported by: Offereins Owned by: hnla
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Templates Keywords: has-patch 2nd-opinion
Cc: lmoffereins@…

Description

The buddypress/groups/single/admin.php file handles all the separate group admin (sub) pages. With the latest inclusion of the cover image page, this has now amounted to a file of almost 500(!) lines. Please can we split this into smaller pieces of separate template files, so that overriding these in a theme gives less concerns with BP updates.

Attachments (6)

7079.patch (35.4 KB) - added by Offereins 20 months ago.
Split /groups/single/admin.php into separate template files
7079-01.patch (56.7 KB) - added by hnla 17 months ago.
-01 patch re-builds original patch taking into account recent heading changes so re-freshes & updates.
7079.02.patch (36.4 KB) - added by r-a-y 17 months ago.
7079.03.patch (37.8 KB) - added by hnla 17 months ago.
Updates -02 witt suggestion to move hidden input out of template to add_action in groups-functions.php
7079.04.patch (37.8 KB) - added by dcavins 17 months ago.
Standardizes indents of .03 patch. Moves hidden input output action to bp-groups-actions.php.
7079.05.patch (38.8 KB) - added by dcavins 17 months ago.
Move hidden input output hook into bp-legacy/buddypress-functions.php

Download all attachments as: .zip

Change History (36)

#1 @boonebgorges
20 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Seems like a good idea to me. Maybe put them in an admin or manage subfolder.

@Offereins
20 months ago

Split /groups/single/admin.php into separate template files

#2 @Offereins
20 months ago

  • Keywords has-patch added; needs-patch removed

Well, here you have it, directly split into /groups/single/admin/ files :)

#3 @DJPaul
19 months ago

  • Component changed from Appearance - Template Parts to Templates

#4 @DJPaul
18 months ago

How about we roll this approach into BP-Nouveau?

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


18 months ago

#6 @hnla
18 months ago

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

We have rolled this into the new Template Pack already as part of a review of all templates in respect of simpler includes and re-usuable code snippets.

As we have a patch here and the changes ought to be non destructive in terms of styles/layouts I'll check the patch over and look to commit it for legacy too in this instance.

#7 @mercime
18 months ago

  • Keywords needs-refresh added; has-patch removed

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


18 months ago

@hnla
17 months ago

-01 patch re-builds original patch taking into account recent heading changes so re-freshes & updates.

#9 @hnla
17 months ago

  • Keywords has-patch needs-testing added; needs-refresh removed

New patch re-builds on original.

Tested across all sub screens to be working on trunk install.

We can commit when we get one other confirmation all works ok!

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


17 months ago

#11 @hnla
17 months ago

  • Milestone changed from Future Release to 2.7

@r-a-y
17 months ago

#12 @r-a-y
17 months ago

  • Keywords needs-testing removed

02.patch changes the /groups/single/admin.php template so we do not have to do so many if conditional statements.

Instead, I've done this:

<?php if ( bp_is_group_admin_screen( bp_action_variable() ) ) : ?>

        <?php bp_get_template_part( 'groups/single/admin/' . bp_action_variable() ); ?>

<?php endif; ?>

In the same file, one thing that I haven't changed, but probably should is this:

<?php /* This is important, don't forget it */ ?>
        <input type="hidden" name="group-id" id="group-id" value="<?php bp_group_id(); ?>" />


I know this is probably a remnant from the older bp-default templates, but since we deem this hidden input field important, we probably shouldn't leave it in the template and should hook it to either 'groups_custom_edit_steps' or 'bp_after_group_admin_content' instead.

It's not that big of deal if we do nothing here, since bp-nouveau is going to take over soon :)

Last edited 17 months ago by r-a-y (previous) (diff)

#13 @hnla
17 months ago

@r-a-y That's a nice bit of simplifying to reduce to one template check line.

I've run with your suggestion to move the hidden input out and added it to groups-functions.php hooked on the after_content action. (not sure if that's best position etc etc).

.03 should be a complete patch (your admin.php changes were based on the earlier or an earlier version so was ?? hunked(2) so just updated my version manually for your changes.

If we can pass off or improve the hidden input change lets then commit and be done! :)

@hnla
17 months ago

Updates -02 witt suggestion to move hidden input out of template to add_action in groups-functions.php

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


17 months ago

#15 @dcavins
17 months ago

This is so much easier to read when it's not monolithic, and will be much better for developers that need to override only one of the screens. Previously, it was a lot of work keeping up with the changes to admin.php if all you wanted to do was make a tiny change on one screen.

The indentation seems kind of arbitrary. Can the sub templates have no base indentation? (I'd be happy to refresh the patch if we're in agreement.)

Thanks for putting this in order! Let's get this committed so we can set about refreshing dependent patches.

#16 @hnla
17 months ago

Yes lets get this committed, we can spend too long back and fourth re-freshing patches ad nuaseam at times. :)

Not too concerned with indentation so yes if you feel it's better aligned to left to begin go for it ( in fact sure you're right there really).

Can we also confirm we're happy with the add_action move in -03, think it's ok but it was a suggestion so I made best stab at a position to move it to which may not be optimal.

@dcavins
17 months ago

Standardizes indents of .03 patch. Moves hidden input output action to bp-groups-actions.php.

#18 @dcavins
17 months ago

  • Keywords 2nd-opinion added

I've added a new patch that standardizes indents and spaces. I've moved the template action (the group ID hidden input outputter) to bp-group-actions.php which doesn't feel awesome, but I don't think it should go in functions because it's not really something that a dev would call. It's like we need a template-actions file. (But we don't really.)

I think it's ready to commit, once someone else has had a chance to sign off on it.

Thanks for this change; it's a really nice improvement for devs!

#19 @hnla
17 months ago

@dcavins Thanks, yep know what you mean with that hidden input it doesn't really live anywhere that makes sense.

#20 @dcavins
17 months ago

One more thought: These screens are wrapped completely in a form, which would make adding a search box in #6385 a bad/impossible idea. (Nested forms aren't allowed, and generally the closure of the inner form is interpreted as the outer form's closure because the inner form's opening tag is ignored.) We have two action points, bp_before_group_admin_content, and bp_after_group_admin_content, but both are contained within the form. Could we add an action point before the form, like bp_before_group_admin_settings_form , for items that need to exist on the screen but not be included in the form?

Otherwise, I guess we could move the <form> tags (and those inside-the-form action points) into every sub template, so those templates can add items outside the form, but that's so much duplication.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


17 months ago

#22 @dcavins
17 months ago

Last chance to dance on this ticket. @hnla is going to commit this in about 18 hours, so please voice your objections now. :)

#23 follow-up: @r-a-y
17 months ago

I've got no issues.

Would like a core dev to comment on what I mentioned in comment:12 about the hidden input field and moving that out of the template.

#24 in reply to: ↑ 23 @dcavins
17 months ago

Replying to r-a-y:

I've got no issues.

Would like a core dev to comment on what I mentioned in comment:12 about the hidden input field and moving that out of the template.

Hi @r-a-y- In https://buddypress.trac.wordpress.org/attachment/ticket/7079/7079.04.patch, I've moved that hidden input out to bp-groups-actions.php. Does that seem like a reasonable place to you for it to go?

I agree that it shouldn't be left in the template--your idea of adding it via an action is a really good one, in my opinion.

#25 @r-a-y
17 months ago

I've moved that hidden input out to bp-groups-actions.php. Does that seem like a reasonable place to you for it to go?

Oops, sorry. I glanced the last message of the thread without catching up! :)

04.patch looks good.

Maybe prefix the function with bp_. Starting the function with groups_ is an old relic from the v1.0-1.1 days I believe.

Hook could also go into bp-legacy's buddypress-functions.php. I just took a look at what bp-nouveau is doing and they are hardcoding the hidden input field directly:
https://github.com/buddypress/next-template-packs/blob/afb886bde7f485e475d79a66afd23c9c490547fb/bp-templates/bp-nouveau/includes/groups/template-tags.php#L434

#26 @dcavins
17 months ago

@r-a-y: I like the idea of the hook going in the template pack. That makes sense, unlike what I had done before.

Attaching a new patch. Thanks for your feedback!

@dcavins
17 months ago

Move hidden input output hook into bp-legacy/buddypress-functions.php

#27 @hnla
17 months ago

@r-a-y :)

I had moved the hidden input on your suggestion in my 02 patch comment #13

Function name was my bad and did think of locating it in buddypress-functions.php but thought better of but why I wanted your feedback on comment:12

All looks good though so lets commit!

#28 @hnla
17 months ago

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

In 11050:

Templates Legacy: Single Groups Admin Screens

Group admin template managing the settings & create steps sub screens has become a file which is hard for developers to navigate through to edit.

Commit re-factors the admin.php file to run as a primary include file checking for the current action variable to load the required template part.

Commit breaks out the sections representing settings /creation steps from the admin template to their own template include files located in a new directory 'single/admin/'.

Smaller split template files are now easier to manage & read for editing and overloading.

Commit re-locates admin files hidden form input to buddypress-functions.php hooking to the existing templates do_action function.

Thanks to Offereins for the original ticket - this commit follows closely the approach used in bp-nouveau templates.

Props offereins, dcavins, r-a-y

Fixes #7079

#29 @hnla
17 months ago

In 11051:

Templates Legacy: Single Groups Admin Screens

Add missed un-versioned new template include files for the admin screens.

Fixes #7079

This ticket was mentioned in Slack in #buddypress by mercime. View the logs.


17 months ago

Note: See TracTickets for help on using tickets.