Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

#7157 closed enhancement (fixed)

UI to pick Template Packs

Reported by: djpaul's profile DJPaul Owned by:
Milestone: 2.9 Priority: strategic
Severity: normal Version:
Component: Administration Keywords: needs-patch good-first-bug
Cc:

Description

Let's build some sort of UI to pick between Template Packs.

Lots of scope for ideation and exploration of UI concepts here, but let's have it as a goal to not confuse people with this screen and WP's Select Theme screen.

The UI needs to be capable of displaying multiple active Template Packs, should we decide to support multiple template packs in the future. This doesn't need to be in the first version, but we can at least come up with a rough plan for this possible future change.

Attachments (2)

7157.patch (39.0 KB) - added by imath 8 years ago.
7157.02.patch (19.4 KB) - added by imath 8 years ago.

Download all attachments as: .zip

Change History (30)

#1 @DJPaul
8 years ago

  • Component changed from Core to Administration

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


8 years ago

#3 @Offereins
8 years ago

One thing that comes to mind - though there is probably a better place to discuss this - is the potential confusion this can give when a Theme has its own template files in a /buddypress folder. Any template pack would then be overridden, though not all template files per se. How should we and/or the theme authors deal with this?

Last edited 8 years ago by Offereins (previous) (diff)

#4 @hnla
8 years ago

As happens now though the files are overridden in part or in full so there would just need to ensure that the pack selection sets the appropriate pack name so BP knows where to look in the template stack for files, I think this is not a major issue.

Not sure this discussion isn't better served on the template pack github issues, as this picker is built as such but it's just residing in the TP plugin for the moment.

#5 @imath
8 years ago

As @hnla said, we already have a "template pack switcher" for the BP Nouveau Template pack. It's pretty simple and the confusion is not possible with themes as it resides in a 4th tab of the BuddyPress settings area.

It can displays any template pack if they exists in WP_CONTENT/bp-templates, the one that is bundled with the plugin and the BP Legacy template pack.

So it can be a start. But considering UI, we should also consider a few things i've met while building some of the features of the next template pack we're working on :

1. How a template pack is listed into this UI ?

Template pack are not themes or plugins, so we can't rely on WordPress.org? Should we provide a way to upload a zip archive of the template pack and add WP_CONTENT/uploads/buddypress/bp-templates to the possible locations?
End users should have a lot of choices between template packs, we can't build everything, should we allow others to be listed into this UI? How can they be referenced a flat text file/posts on BuddyPress.org?? How are we importing the zip archive? link to github?

2. How a template pack is upgraded ?

So far BP Legacy was upgraded with BuddyPress. But we won't be able to bundle all template packs into the plugin in the future. If there's something to do, the UI must take it in account and do HTTP requests to see if an upgrade is needed for example.

3. How a template pack deals with i18n ?

BP Legacy uses the 'buddypress' text domain, but any other like BP Nouveau must use another domain. So where these pot/po/mo files should be located, how they will be generated (< for this i've extended the WordPress tool to generate specific pot files, because again Template Pack is not a plugin or a theme), How they will be used to be translated by the community? Again the UI must take this in account because the list can't just be a list of names, it must include a description and ideally not in english but in the native language of the end user... (for the template pack switcher we thought about some additional columns like the supported components, or feedback messages like BuddyPress/WordPress required versions). But i think they should include at least a screenshot/ a link to more informations about the template pack etc...

These are another 3 important points this UI will need to deal with :)

Don't hesitate to test The next-template-packs plugin that is including the BP Nouveau Template pack, you'll have a nice overview of a possible very simple UI :)

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


8 years ago

#7 @DJPaul
8 years ago

I like the design of the template pack chooser in the nouveau stuff. Let's get that in.

#8 @DJPaul
8 years ago

In reply to your questions, I think you are overcomplicating things. I'm not aware that *anyone* has ever just shipped a replacement template pack for BuddyPress. Worrying about BuddyPress maybe not wanting to bundle all future template packs is premature.

  1. How a template pack is listed into this UI? ... Template pack are not themes or plugins

Why not? I think a plugin is a great distribution mechanism for a 3rd party template pack author. It solves all the problems you mentioned.

#9 @imath
8 years ago

I agree, i often overcomplicate :)

You're probably right about it.

I'll work on a patch to bring the Nouveau UI switcher into core.

#10 @imath
8 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 2.7

7157.patch contains my suggestions for the UI to switch template packs.

It's actually doing a bit more.. So i'll explain with screenshots:

https://cldup.com/iv1BIt_MHL.png

This is how the UI will look by default. As we only have one template pack in core. So i thought to "ease" BP Nouveau testing we could let users upload new template pack. When you click on the upload button, you get a form to upload and install a zip archive of a Template Pack.

https://cldup.com/jBcSgAPgZl.png

I've discovered that Using the file system in WordPress requires the same credentials wherever you want to unzip a file. I thought it was not requesting any credentials in uploads for instance, and it's actually the same than unzipping into /wp-content/bp-templates.

I wasn't able to test completely the "credential" form WordPress is outputting when FTP username and password are required. So i choose to display an error message to inform the user he needs to use FTP. If you can directly upload new plugins, then it should work for Template Packs.

Once installed, you get:
https://cldup.com/D6hgBOUJip.png
From there, you can delete the uploaded template packs, activate the one of your choice and even preview a Template Pack before activating it.

@imath
8 years ago

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


8 years ago

#12 @DJPaul
8 years ago

  • Keywords dev-feedback added

I really do not see the need to let people upload these templates. As I said before, a plugin is a simple way to achieve distribution and maintenance/updates (along with all the other benefits of being a plugin). I think this is very over-engineered.

We need a second opinion from @boonebgorges or @jjj here because I would hate for you to spend time more time building something complex that we don't end up using.

#13 @boonebgorges
8 years ago

I really do not see the need to let people upload these templates. As I said before, a plugin is a simple way to achieve distribution and maintenance/updates (along with all the other benefits of being a plugin).

I think I agree with this. A plugin seems more straightforward, especially since the construction of an entire template pack is a large task that few are going to do in practice.

Regarding "location" registration: I think it's fine to have a standard location in buddypress/bp-templates for our packaged templates. But I think it's overkill, and not very transparent for third-party developers, to have to follow a specific standard for directory naming in order to have their theme templates recognized. An explicit function for registering a template pack seems best. I see that the function bp_register_theme_package() is called in 7157.patch, but is not defined there - is the patch incomplete? In any case, a function like this seems like a much more straightforward way to have plugins declare that they contain a template pack.

What's the best practice for themes that come with a complete set of BP templates? Should they be registering template packs? If you're running bp-nouveau TP with twentysixteen, but then you switch to a theme that contains a complete /buddypress/ directory (which is a TP, except it's not "registered"), what should happen? This is related to the question that @Offereins asks above.

The picker UI seems fine for a first pass. If we start shipping multiple template packs, it'd be nice for the picker to be a bit more visual - maybe something closer to the WP theme picker experience, with large screenshots/previews.

#14 @johnjamesjacoby
8 years ago

I agree that for this first pass, not having an "Upload" UI does make sense. This first step is introducing the concept visually, and maybe next steps once there are more than a few packs available, will be to allow for some kind of easier interface for "installing" them (which is really just uploading and activating a plugin.)

But I think it's overkill, and not very transparent for third-party developers, to have to follow a specific standard for directory naming in order to have their theme templates recognized

This is more indicative of a shortcoming of how WordPress's template hierarchy works, than it is about how BuddyPress needs to work.

An explicit function for registering a template pack seems best. I see that the function bp_register_theme_package() is called in 7157.patch​, but is not defined there - is the patch incomplete? In any case, a function like this seems like a much more straightforward way to have plugins declare that they contain a template pack

bp_register_theme_package() is already a core function, but it also just uses the BP_Theme_Compat class which was purposely ambiguous and terrible so we could narrow what it needed and would be later.

What's the best practice for themes that come with a complete set of BP templates? Should they be registering template packs?

Good question. It's ultimately up to theme author to decide if they are owning the entire experience without fallbacks, or if they are introducing a template pack that fits into a stack of template locations to look for. I think the smart default is for themes to not announce themselves as template packs, and for template packs to only come in plugin form.

The picker UI seems fine for a first pass.

100% in agreement. <3

#15 @hnla
8 years ago

What's the best practice for themes that come with a complete set of BP templates? Should they be registering template packs?


Good question. It's ultimately up to theme author to decide if they are owning the entire experience without fallbacks, or if they are introducing a template pack that fits into a stack of template locations to look for. I think the smart default is for themes to not announce themselves as template packs, and for template packs to only come in plugin form.

I would agree here, the distinction is template packs would tend to be user (non coder) focussed and as such a plugin makes sense. A theme ( or as I prefer to see things a developer and custom site build) that has opted to provide support for BP would by almost default override all files to the site themes directory and thereafter provide all the necessary cuastom styles & manipulating the theme_compat class if necessary too.

If you're running bp-nouveau TP with twentysixteen, but then you switch to a theme that contains a complete /buddypress/ directory (which is a TP, except it's not "registered"), what should happen?

The user deactivate the TP plugin? In an automagic sense though perhaps we need to only allow TP to activate in a theme that hasn't overloaded template files or instantiated a new theme_compat class, perform some of the checks that core does to establish what a sites files are doing. So switching to a theme already handling BP files would cause any activated TP to deactivate, if the next question is how does the user grasp what's happening it's no different to a plugin notice of deactivation, hopefully the theme would be making it clear it provides it's own BP experience that overrides any TP activated.

#16 @imath
8 years ago

Thanks for your feedbacks.

7157.02.patch is:

  • removing the upload/delete part of the patch.
  • simply listing BP Legacy and the active plugins who registered a theme package using the existing bp_register_theme_package() function.
  • keeping the "Preview" and "Customize" buttons.

But I think it's overkill, and not very transparent for third-party developers

The developer can use any location of his choice as soon as he respects this organisation:

/anyrandomname
   /buddypress
   /css
   /js
   buddypress-functions.php

what should happen? This is related to the question that @Offereins asks above.

I don't know about "should". But Theme designers can eventually:

  • build a Standalone BuddyPress theme using add_theme_support( 'buddypress' ). Just like BP Default.
  • Force bp_get_theme_compat_id() to return 'legacy'

@imath
8 years ago

#17 @DJPaul
8 years ago

bp_core_admin_theme_packages_get_head

  • The array_fill_keys strikes me as unnecessarily complex and harder to read, considering the content is static.
  • The PHPDoc indicates a return type, but this function prints text to output.
  • The various printf parameters need appropriate escaping. I know the various column names and values are static, but they should be escaped to make sure they will always be safe in future (i.e. if someone adds something and doesn't read the rest of the function correctly).

bp_core_admin_theme_package_metas

  • The PHPDoc indicates a return type, but this function prints text to output.
  • Why is $theme_package_id an optional argument, if no default value is provided inside the function?

bp_core_admin_theme_package_get_warnings

  • PHPDoc says it returns a string, but it actually returns an array.
  • "This component is often forgotten" -- as wikipedia says in English: citation needed. :) Where/how? Some reasoning for this assumption would be useful, thank you.

bp_core_admin_theme_packages_get_body

  • The PHPDoc indicates a return type, but this function prints text to output.
  • On the line starting <tr id="<?php echo esc_attr( $theme_package->id ); ?>", for the class, please concatenate the strings *then* esc_attr. Don't esc_attr twice and then glue them together.
  • Some inline CSS, can we move to one of the admin stylesheets?
  • Empty span in the plugin-title block. And row-actions-visible. Suspicious.
  • That check-column th is also suspiciously empty. Maybe copy what WordPress does -- I will be surprised if it should be left blank.
    • I imagine @mercime will be happy to help iterate on this form for accessibility guidance once the first version is committed, so don't worry too much yet.
  • On the line starting <label for="bp_theme_package-, please esc_attr for the entire for attribute value. i.e. move "bp_theme_package-" inside the esc_attr and concatenate it there.
  • I think the list of warning in the attntion div should not be split by <br/>. I think we had an accessibility change recently where we swapped this. They should each be <p>, or perhaps a <ul>.

bp_core_admin_theme_packages

  • Some inline CSS, can we move to one of the admin stylesheets?

Apart from all of this, I think the function names are too long (not your fault, but we don't have to copy what we've done before if we can do it better now).

Please only use _get_ in function names if they return a value. That's the pattern we try to keep to throughout the project.

And, why did you decide to put all this new code in a new file? Is splitting the other admin UI screens into their own files an approach you think we should do (in the future)?

#18 @mercime
8 years ago

Replying to DJPaul:

I imagine @mercime will be happy to help iterate on this form for accessibility guidance once the first version is committed, so don't worry too much yet.

First pass. Worked in chrome browser and took some notes of initial check:

  • Need to move <tfoot>...</tfoot> to afer </tbody> in markup to improve accessibility of content. See #7065.
  • Remove extra and empty <thead> and <tfoot> markup showing up in source code following each <thead> and <tfoot> respectively.
  • Add wp-list-table to table class attribute to enjoy the list table responsive goodness.
  • Add column-primary to class attribute of the 2nd column of each row for responsive goodness to work.
  • Screenshot of first pass working it in the browser https://cldup.com/cJtExK9BxO.png which has additional information

Note: excuse the change between input type="radio" and input type="checkbox" in above screenshot of source code and the image below. The checkbox for <thead> and <tfoot> and radio input for the template pack row.
https://cldup.com/FVpcyYR21y.png

Some inline CSS, can we move to one of the admin stylesheets?

+1 Move styles to admin stylesheet.

Empty span in the plugin-title block. And row-actions-visible.

The span can be removed unless you want to add a dashicon, SVG, or image, etc. The div for row-actions-visible can be removed as well if not planning to add anything there.

That check-column th is also suspiciously empty. Maybe copy what WordPress does -- I will be surprised if it should be left blank.

I added a disabled checkbox in first column of <thead> and <tfoot> to remove validation warnings. e.g. in recent Components screen update.

#19 @DJPaul
8 years ago

🎉

#20 @mercime
7 years ago

  • Milestone changed from 2.7 to Future Release

#21 @DJPaul
7 years ago

  • Keywords needs-patch good-first-bug added; has-patch dev-feedback removed
  • Milestone changed from Future Release to 2.8

Let's get this in for 2.8 given its usefulness and the amount of work spent on it. There's a lot of feedback regarding the existing patch, so a good patch for anyone to dive into.

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


7 years ago

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


7 years ago

#24 @hnla
7 years ago

  • Milestone changed from 2.8 to Future Release
  • Type changed from defect (bug) to enhancement

Changing this to 'future release' with the intention we move it to 2.9 as soon as that cycle begins and again push for an early implementation.

#25 @mercime
7 years ago

  • Milestone changed from Future Release to 2.9

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


7 years ago

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


7 years ago

#28 @DJPaul
7 years ago

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

This was done in r11593

Note: See TracTickets for help on using tickets.