search

buddypress / next-template-packs

is this the next BuddyPress template pack?
35 stars 9 forks source link

Move non-core template parts in /assets/ directory to somewhere else #23

Closed r-a-y closed 8 years ago

r-a-y commented 8 years ago

I think this directory: https://github.com/buddypress/next-template-packs/tree/master/bp-templates/bp-nouveau/buddypress/assets

Should only be used for overriding core functionality like emails and embeds.

So might I suggest that the activity, invites and messages directories be moved to buddypress/common or includes or some other directory?

imath commented 8 years ago

I disagree :)

Originally this directory was used to include JS templates for the avatar/cover images feature. into the directories you're mentioning these are JS Templates too.

And by the way why is it annoying ?

imath commented 8 years ago

mmm i see, there could be a problem if some directory names/template names were similar with the BP Legacy template pack. A good way to avoid this is to prefix the templates. Is this ok for you ?

r-a-y commented 8 years ago

My main reason was because of https://buddypress.trac.wordpress.org/ticket/7116.

My thinking is the /assets/ directory is something that should only reference core templates if 7116 is committed. However, there is no way to enforce this, so I'm open to some suggestions here.

A good way to avoid this is to prefix the templates. Is this ok for you ?

Can you give an example?

It seems that the extra templates in /assets/ are Backbone templates. Perhaps something like /includes/backbone/ or even /buddypress/js-templates/ might make sense here?

imath commented 8 years ago

I'm a bit confused here. What are core templates ?? these are BP Legacy templates, so it's a bit weird to use "core templates" If assets were a private directory it should have been prefixed with '_'.

The only trouble i can think of would be if a template like assets/activity/form.php was added in BP Legacy to do something else than what we're doing in BP Nouveau.

So my suggestion is to rename the templates we're using into the assets directory by prefixing them with bp-nouveau, so our assets/activity/form.php would become assets/activity/bp-nouveau-form.php. This should prevent any template misuse. I doubt Legacy would ever use 'bp-nouveau' for their template names ;)

Because by the way no matter what we're doing in core Your comment in 7116 showed that if we don't include the assets/emails folder, the legacy template or the "core template" is not loaded ;)

And if you move up the assets into bp-templates, then our bp-nouveau/buddypress/assets folder is not a problem anymore :)

r-a-y commented 8 years ago

What are core templates ??

Core templates is a concept that I'm proposing in 7116. So you're right that it isn't really applicable at the moment. I'm just thinking ahead :)

These templates would reside in /bp-templates/assets/ (we could change this directory name if desired) and would load after theme directory checks and template pack checks. So if the theme or the template pack does not include these template parts, we would load the default template part from /bp-templates/assets/.

Your comment in 7116 showed that if we don't include the assets/emails folder, the legacy template or the "core template" is not loaded ;)

7116 would fix this :)

If assets were a private directory it should have been prefixed with '_'.

Yeah, in hindsight, that was an issue we should have thought about.

So my suggestion is to rename the templates we're using into the assets directory by prefixing them with bp-nouveau, so our assets/activity/form.php would become assets/activity/bp-nouveau-form.php. This should prevent any template misuse. I doubt Legacy would ever use 'bp-nouveau' for their template names ;)

Prefixing filenames in a theme or template pack seems weird to me.

Is it necessary for these JS templates to live in assets?

I guess you're thinking that since the assets/_attachments directory has JS templates that it makes sense for them to live in the assets directory as well. I can see your point here as well.

hnla commented 8 years ago

Prefixing filenames in a theme or template pack seems weird to me.

Yes lets avoid this if at all possible.

imath commented 8 years ago

This is becoming completely crazy! But it's interesting!

Yes it has to be in assets else why do we have JS templates in _attachements ?

And @r-a-y the interesting part here is you are kind of saying BP Nouveau will never be included into core so don't use "BP Legacy" locations. Is that what you're saying ? Because else this question would never have raised.

If that is the case my precaution of always prefixing template tags or functions used in BP Nouveau is a good practice!

Why are we using bp-nouveau/buddypress/activity/index.php, maybe we shouldn't! If we put the assets directory into the template pack that was to allow overriding.... But now we shouldn't anymore ??

And by the way i'm pretty sure plugins and themes are already putting things there...

r-a-y commented 8 years ago

And @r-a-y the interesting part here is you are kind of saying BP Nouveau will never be included into core so don't use "BP Legacy" locations. Is that what you're saying ? Because else this question would never have raised.

No, what I'm saying is if you need to override something like the attachments, emails or embeds templates for further customization, you would add it in the template pack's /assets/ folder.

If you do not need to override these templates, don't add them in your template pack because a fallback template will always be provided by core if 7116 is committed. For example, does bp-nouveau really need to add a new email template? If not, don't add it.

This is what I'm proposing in 7116 for asset template loading:

  1. Try to find template in theme directory. If found, load it.
  2. Try to find template in current template pack directory (whether this is bp-legacy or bp-nouveau or whatever). If found, load it.
  3. No template found, so load default template. This is the new addition that 7116 is trying to solve.

If we put the assets directory into the template pack that was to allow overriding.... But now we shouldn't anymore ??

As bp-nouveau has shown, there is no way to tell how people are already using the assets directory, so I'm not sure if what I'm proposing is making any sense or not.

I'm just saying that if 7116 goes through, it will be hard to differentiate between core template parts and whatever custom templates bp-nouveau is using in the assets directory. I guess that is what prefixing the filenames is trying to solve, but it's just a little weird IMO.

If you're going to be prefixing the filename, why not use a directory instead? eg. /assets/js-templates/

imath commented 8 years ago

And one of these days /assets/js-templates/ this location will become core. I really don't understand the problem. But i must say i don't care that much to carry on arguing. So i'll create a new directory with a french name to be sure core will never use it!

r-a-y commented 8 years ago

It's not really an argument. More of a friendly discussion! :)

I would say leave things be for the moment. Don't change anything yet.

imath commented 8 years ago

too late :) It's not a big deal anyway.

Sorry i misused "arguing" i was thinking of "argumenter" in french which means discussing different point of views and trying to convince.