Skip to:
Content

Opened 3 years ago

Closed 3 years ago

#3389 closed defect (bug) (fixed)

Enqueueing CSS of bp-default breaks child theme layouts

Reported by: mercime Owned by:
Milestone: 1.5 Priority: normal
Severity: normal Version:
Component: Theme Keywords: has-patch
Cc:

Description

BP 1.3 trunk-4863 and WP 3.2.1

Child theme layout breaks when default.css is enqueued in theme's functions.php and adminbar.css is enqueued via /bp-core/bp-core-adminbar.php. Styles in both override child theme styling.

Some suggestions:

  1. Consider reverting back to adminbar.css and default.css imported via style sheet.
  2. Add warning in one of the panels during upgrade to BP 1.3 about styling and also about change of location for the adminbar.css for importing.
  3. Add option to disable enqueueing BP 1.3 default and/or adminbar style/s

Thank you.

Attachments (11)

BPSNetwork-layout-broken.jpg (74.8 KB) - added by mercime 3 years ago.
Child theme layout breaks upon upgrade to BP 1.3 trunk
BPSNetwork-layout-should-be.jpg (75.9 KB) - added by mercime 3 years ago.
Actual child theme when bp-default styles are disabled
unplugged-what-happened.gif (54.1 KB) - added by mercime 3 years ago.
Unplugged child theme upon activation
unplugged-even-with-deregis.gif (55.6 KB) - added by mercime 3 years ago.
Unplugged child theme even with deregister script in new functions.php
BPSNetwork-with-deregister.gif (39.0 KB) - added by mercime 3 years ago.
BPSNetwork theme even with deregister script
BPSNetwork-debogger.gif (47.3 KB) - added by mercime 3 years ago.
Using Debogger Plugin
3389.01.patch (805 bytes) - added by boonebgorges 3 years ago.
3389.02.patch (1.8 KB) - added by boonebgorges 3 years ago.
3389.03.patch (1.6 KB) - added by boonebgorges 3 years ago.
3384.04.patch (1.5 KB) - added by r-a-y 3 years ago.
3384.05.patch (808 bytes) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (61)

mercime3 years ago

Child theme layout breaks upon upgrade to BP 1.3 trunk

mercime3 years ago

Actual child theme when bp-default styles are disabled

comment:1 DJPaul3 years ago

Is the theme you are testing with available for us to try with?

comment:2 r-a-y3 years ago

  • Component changed from Core to Theme

@mercime - Old skool! ;)

How are you enqueueing your stylesheet? Are you trying to outright remove the default theme's stylesheet? If so, you should be able to remove the default one using wp_deregister_style during the after_setup_theme action.

Or you might need to use the wp_deregister_style inside the wp_print_styles action. Action firing!

eg.

function my_print_styles() {
     wp_deregister_style( 'bp-default-main' );

     // enqueue your style now
     wp_enqueue_style( 'my-bp', get_stylesheet_uri(), array(), BP_VERSION );
}
add_action( 'wp_print_styles', 'my_print_styles', 20 );

Also, make sure you don't have a hardcoded stylesheet in your theme's header.php.

Last edited 3 years ago by r-a-y (previous) (diff)

mercime3 years ago

Unplugged child theme upon activation

mercime3 years ago

Unplugged child theme even with deregister script in new functions.php

mercime3 years ago

BPSNetwork theme even with deregister script

mercime3 years ago

Using Debogger Plugin

comment:3 mercime3 years ago

@djpaul @ray it would be better if I give another example of what enqueueing CSS in bp-default theme would do to other installs using the Unplugged BP child theme with CSS-ONLY revisions http://wordpress.org/extend/themes/unplugged

You see the all-dark Unplugged theme upon activation turns white in content area per image above. Adding the just the deregister default.css function without enqueueing own style in a new functions.php I uploaded to Unplugged theme folder is not working as seen in second unplugged image and in image from theme I am working on for submission to WP Theme Repo.

As for your code @ray where in addition to deregistering default.css enqueueing my own style as well, it's working great. But I doubt everyone else upgrading their installs to BP 1.3 would think or know outright that they would need to deregister default.css and to enqueue own styles in the first place.

It looks like if one doesn't use the exact selector used in default.css of bp-default theme, the CSS of child theme still gets overwritten especially in BP component areas even if one already uses wp_deregister_style, which shouldn't ideally be the case.

Two ways I could completely prevent default.css from enqueueing were as you can see, not quite ideal but has allowed me to continue working on the BP child theme:

  • comment out the function in bp-default functions.php
  • or add the following to child theme's functions.php
    function bp_dtheme_enqueue_styles() {
    	// leave it blank
    	}
    	add_action( 'wp_print_styles', 'bp_dtheme_enqueue_styles' );
    

@djpaul, the theme is not fit for distribution yet. Working on issues brought up by Debogger plugin per BPSNetwork-debogger.gif attached.

I haven't checked what happens to installs using WP themes with BP Template Pack, but I will venture to say that there will be a great influx of users into BP.org forums who upgrade to BP 1.3 and see layouts broken in BP Child Themes alone. Which brings me back to suggestions above or find better alternative please.

For your consideration.

Last edited 3 years ago by mercime (previous) (diff)

comment:4 hnla3 years ago

This came up a while back when that Paul bloke went and enqueued all the CSS, broke my dev theme, but I simply de-registered bp-default styles without any problem although the suggested way didn't work for me.

I can see the issue with themes that worked of @imports and files called from child theme structure though.

comment:5 karmatosed3 years ago

*Chucks in 2 cents... The problem is the existing themes really more with regard to this. I agree with Mercime and we potentially have a ton of themes that won't work once the move to 1.5 happens. There may be a few themes that get updated but if they do not the theme will simply stop working.

Whilst not a perfect option perhaps we need to go the FAQ route here.

I agree enqueue is the way forward. However, not all theme developers will be either aware or going over them so that leaves a gap of users who are going to find themes falling over (last thing we need is something to put people off BuddyPress themes).

If there is no other solution perhaps we can do as suggested the FAQ route and at least have steps in there we can point to. I also think a blog post / bpdev post now would be apt during beta to try and poke some theme peeps into action.

Last edited 3 years ago by karmatosed (previous) (diff)

comment:6 hnla3 years ago

@karmatosed this will be the only real option, adding a clear guide to dealing with this issue in the codex, sadly even doing that is still going to result in a lot of firefighting of user issues with child themes built, used, but no longer supported, with users not versed in editing core files.

This is another reason (although I haven't thought it through in huge detail :) )why I think it might have been better to not change bp-default too much, essentially regarding it as a retired theme left alone for in all but necessary changes to core files and to push ahead with a version II new bp-default can two themes exist in bp plugin and be selectable by a child theme style file ?

P.S. This ticket/thread was way older than 11 days; have the time stamps been messed up in the shuffle?

Last edited 3 years ago by hnla (previous) (diff)

comment:7 boonebgorges3 years ago

Ran across this problem today. Did we ever settle on best practices? I'm using mercime's suggestion of overriding bp_dtheme_enqueue_styles(), but this seems quite hackish indeed.

comment:8 hnla3 years ago

Actually it was me that originally proposed the de-register styles as the only option that I could find that worked around the issue in this thread:
http://buddypress.org/community/groups/creating-extending/forum/topic/theme-development-on-trunk/

Haven't really looked at things since, assuming somewhat that it was only solution to hand. I modified things to:

add_action( 'wp_print_styles', 'deregister_bp_dtheme_styles', 100 );
function deregister_bp_dtheme_styles() {

wp_deregister_style( 'bp-default-main');
}

comment:9 r-a-y3 years ago

I was thinking if there was a simple way to detect if a child theme was older than 1.5, we could add some backpat compatibility.

One way I was thinking about was output buffering (output buffering would be too late) reading a child theme's header.php and seeing if stylesheet_url or bp_page_title() was in the code. If detected, don't enqueue 1.5 styles.

If there's a simpler way than that, that would be preferred.

Last edited 3 years ago by r-a-y (previous) (diff)

comment:10 karmatosed3 years ago

I'm not sure that will catch all some enqueue but still would be broken as a result of this. However it's I still think a case of education.

I just did a basic test run and rather than de-registering used the function like this:

function bp_dtheme_enqueue_styles() {
	// Bump this when changes are made to bust cache
	$version = '20110804';

	// Default CSS
	//wp_enqueue_style( 'bp-default-main', get_template_directory_uri() . '/_inc/css/default.css', array(), $version );

	// Right to left CSS
	if ( is_rtl() )
		wp_enqueue_style( 'bp-default-main-rtl',  get_template_directory_uri() . '/_inc/css/default-rtl.css', array( 'bp-default-main' ), $version );
}
add_action( 'wp_print_styles', 'bp_dtheme_enqueue_styles' );

I put that in the child functions.php and commented out the style to show you can just remove it.

Now this works. You'd just enqueue your styles in that function then if had own.

This still breaks themes but is the way I'd suggest aside from de-register.

Which ever way it's either a hack to get around older themes and requirement for education as hack won't always stay in or education now and then no hack. Brutal maybe but way I see it possibly in a simplistic overview.

comment:11 boonebgorges3 years ago

I was about to come say to r-a-y that output buffering would be too late, but he beat me to it :)

I'm still not sure if I understand what's going on in general. I get that we might just have to have an education campaign for upgrading BP 1.2 themes. But what is the recommended route for new themes from here on out? Will all (past present and future) child themes of bp-default have to manually unregister bp-default's css? And (I have a feeling this is a dangerous question to ask) why are we enqueuing our stylesheet for bp-default anyway?

comment:12 karmatosed3 years ago

Using a function like this is pretty standard now - most themes will have a setup function and others like this.

I'd say it's about getting inline with best practices in part but also it's not a case of having to deregister. In this function you just don't have it called and it won't enqueue. See my above example no unregistering going on. It's actually more flexible if about face and certainly more future proof using the !function_exists check (below code is the call in default):

if ( !function_exists( 'bp_dtheme_enqueue_styles' ) ) :
/**
 * Enqueue theme CSS safely
 *
 * @see http://codex.wordpress.org/Function_Reference/wp_enqueue_style
 * @since 1.5
 */
function bp_dtheme_enqueue_styles() {
	// Bump this when changes are made to bust cache
	$version = '20110804';

	// Default CSS
	wp_enqueue_style( 'bp-default-main', get_template_directory_uri() . '/_inc/css/default.css', array(), $version );

	// Right to left CSS
	if ( is_rtl() )
		wp_enqueue_style( 'bp-default-main-rtl',  get_template_directory_uri() . '/_inc/css/default-rtl.css', array( 'bp-default-main' ), $version );
}
add_action( 'wp_print_styles', 'bp_dtheme_enqueue_styles' );
endif;

My own 'recommendation' and what I'm doing is two fold:

  1. set up theme function (aside point but valid to do)
  2. just call this function and set in what you want from child theme.

For instance in my own parent / child themes I am doing similar for their own styles even outside of BuddyPress Default. It really makes things simpler as you can override any function in the child without deregistering you just don't use if don't want.

I will add I'm doing this a lot so may have rose tinted glasses on how effective it is as part of my practice now so open to both sides of the argument.

Last edited 3 years ago by karmatosed (previous) (diff)

comment:13 hnla3 years ago

Shouldn't really have been done this time round, not just for the sake of getting into line. present theme should have been retired and these sorts of changes and other improvements moved to a new theme ( can bp hold two themes one to preserve older child themes and one that moves forward, don't really see why not)

Karamatosed not really understanding what you're doing? for a true child theme or at least one that is as close to a true theme as BP allows one has to accept functions.php if you have your own styles then you have to do what I'm having to do and deregister the BP enqueueing of the styles.

Enqueueing is a cool feature of WP but more so where scripting is concerned - although I do like the fact I can add query strings, but for CSS I still think it's a relatively low benefit and if you know how to manipulate headers and caching directives you will not lose anything by not enqueueing styles.

comment:14 karmatosed3 years ago

Have you tried declaring that function in the child functions.php? That is what I'm doing declaring it there and you can inherit or not your call... Either just keep as is and add child enqueues, remove and add new own or don't call at all. Why doesn't that work after all there is an exist check around it...

I'm starting to be confused myself why that is an issue so please can you explain as what I use myself and have working in child themes of default also. Why cant you declare that function? There is no need to deregister if you do this.

Last edited 3 years ago by karmatosed (previous) (diff)

comment:15 hnla3 years ago

Your function checks to see if a function exists if not enqueues the files, but the issue is that the files ARE now being enqueued not that they weren't, when they weren't all was ok wasn't it? however ones child theme dealt with calling required styles was ok as they were all called via url paths you set new paths in style.css to the files, with the introduction of enqueued styles you lose some of the control if say you sourced the default adminbar ( I know this has all changed) css from bp-default but not default.css or had moved default css to your own inc folder and modified it then suddenly you have an enqueued default sheet at least this is how one can perceive one of the issues that could arise?

I for example have my own stylesheets no bp-defaults at all but suddenly I have the BP defaults loading which I didn't want and that break my layout into teeny pieces of horribleness.

comment:16 karmatosed3 years ago

Erm it's a function from the default theme not mine and if you use it like I did it won't enqueue. I can replicate it not enqueueing using default and a child. That is the reason it has the function exists check so you can use it and whatever you put in enqueues. That way you are calling those styles as and when you want just enqueing.

Maybe I'm wrong but I don't get issue with enqueuing if you can deal with it so easily defining the function. For your case (which is same as mine for other themes) define function in your theme functions.php (assuming you are child of bp default of course) then remove totally that'd enqueue line for default, whack in your own enqueues for styles... Job done? Is it something you have to change? Yes and that is where education comes in it won't work out of box that is not point I'm making but you can make it work easier than deregistering.

comment:17 DJPaul3 years ago

As mentioned above, DTheme enqueue style is pluggable, so child themes can declare an empty function to prevent these defaults.
The admin bar CSS should never have been included in DTheme, which is why it was moved out.

comment:18 boonebgorges3 years ago

The admin bar CSS should never have been included in DTheme, which is why it was moved out.

Agreed. This makes sense. (Though it will make it tricky to override admin bar CSS in one's theme. You'll have to enqueue an extra stylesheet to be dependent on bp's admin bar CSS.)

I don't have a problem per se with bp-default's stylesheet being enqueued. The pluggable method seems fine to me. We just need to be extremely clear in the documentation about it, since it seems to me (as someone who doesn't work much with WP themes - I may be wrong about this) that this behavior is not standard; I've never seen a theme that has an intentionally blank style.css in favor of enqueuing. (I can see why this standard would be nice, though.)

3389.01.patch has some suggested wording for the inline docs. The BP codex page should be updated tout de suite with a section about this as well.

boonebgorges3 years ago

comment:19 karmatosed3 years ago

I already linked from codex to this but I can add in this information too later today or tomorrow if that works.

http://codex.buddypress.org/releases/1-5-developer-and-designer-information/

comment:20 hnla3 years ago

that this behavior is not standard; I've never seen a theme that has an intentionally blank style.css in favor of enqueuing. (I can see why this standard would be nice, though.)

It seems odd, and isn't - I think - that prevalent in WP themes, If this was the standard so to speak then style.css would be better as something else 'theme_conf.php' rather than loading a stylesheet for what appears no real reason.

With enqueueing I see it as far more applicable to e.g the plugin project I'm working with that has a huge heap of styles and scripts to manage and load and enqueueing is the means to keep all that rationalised and organised loading as and when required and in that sense god bless enqueueing as without it hell on earth would ensue.

comment:21 hnla3 years ago

@karmatosed The function works for those that wish to enqueue styles, for the sake of brevity I suggest we also include a note on how to simply deregister or return an empty function as Paul points out should be done?

comment:22 karmatosed3 years ago

I updated the codex here:

http://codex.buddypress.org/releases/1-5-developer-and-designer-information/

@hnla: I don't actually agree it's newish but it's happening more and more in themes. It's something I've personally used for a while even outside BuddyPress as have others. It really helps with child themes. I think we need to agree to disagree though as becoming a point everyone has opinions on but hopefully we can at least educate and get word out to get it known to all how to work with the default theme.

comment:23 hnla3 years ago

I'm not really debating anything so there's nothing for us to agree or disagree on Tammie.

There's no huge benefit to enqueueing main styles although as I've said elsewhere being able to control load order and dependencies is useful but really more so where large complicated plugins are concerned. But if people are then WP ought to think about the point of style.css as it should not be loaded. you do not load empty files, it rapidly becomes redundant.

comment:24 follow-up: mercime3 years ago

... this behavior is not standard; I've never seen a theme that has an intentionally blank style.css in favor of enqueuing. (I can see why this standard would be nice, though.)

Agree on all points. I may have missed something, I haven't reviewed or seen any parent/framework theme in the WP Theme Repo that has favored enqueueing main stylesheet over all in style.css and/or @import.

A certain someone mentioned to me I have to replace the two outdated BP themes I recommended for suspension in the WP theme repo. I agreed :-) Now who's going to replace all the BP child themes which will be suspended from WP theme repo because of enqueued main stylesheet when BP 1.5 rolls out? :-)
EDIT - someone already marked the BP themes in repo as such http://codex.buddypress.org/releases/1-5-theme-compatibility/

Is the style method set in stone already or are devs still willing to be persuaded to do otherwise?

Last edited 3 years ago by mercime (previous) (diff)

comment:25 in reply to: ↑ 24 ; follow-up: johnjamesjacoby3 years ago

Replying to mercime:

Is the style method set in stone already or are devs still willing to be persuaded to do otherwise?

If someone can dream up a better solution that still enqueues the styles, happy to entertain it. :)

comment:26 johnjamesjacoby3 years ago

  • Milestone changed from Awaiting Review to 1.5
  • Version 1.5 deleted

comment:27 boonebgorges3 years ago

(In [5051]) Adds inline documentation regarding enqueuing methods for stylesheets of bp-default child themes. See #3389

comment:28 boonebgorges3 years ago

  • Keywords close added

If someone can dream up a better solution that still enqueues the styles

I don't think that such a thing exists. If we switch to enqueuing, I don't think there's really a better way to do it than the method already implemented. My own view is that we should use @import for backpat, but I will defer to Paul and John who seem to have stronger feelings about it.

Between the codex documentation and the inline docs, I think that we are about as clear as possible. If we are happy with the trade-off between the advantages of enqueuing and the disadvantages of dealing with questions about how this works, then I suggest we close the ticket as wontfix.

comment:29 in reply to: ↑ 25 mercime3 years ago

Replying to johnjamesjacoby:

If someone can dream up a better solution that still enqueues the styles, happy to entertain it. :)

That's a tough one, JJJ. I can only suggest a compromise == Keep all stylesheets enqueued except for the main stylesheet, default.css for this BP 1.5 version. Allow users to focus on and appreciate the many many new cool features packed in BP 1.5 instead of freaking out when they upgrade to BP 1.5. Have mercy.

comment:30 johnjamesjacoby3 years ago

What if we compare the stylesheet to the template, and if bp-default is both we enqueue, if it's the template, we don't enqueue and let child themes @import our style.css, which will @import our CSS. Fixes everything?

comment:31 boonebgorges3 years ago

jjj - Interesting idea, though in practical terms it means that hardly anyone will ever enqueue the stylesheet, as I would imagine that fairly few installations use bp-default itself. Plus, by not enqueuing 100% of the time, you lose most of the benefits of enqueuing (like the ability for arbitrary plugins to deregister the CSS and reregister with dependencies).

I think we should consider mercime's most recent suggestion, that we enqueue all stylesheets except for default.css for BP 1.5.

comment:32 hnla3 years ago

hate when someone nips and blocks me posting a comment :)

And that would be a happy compromise wouldn't it? Doing something similar on a plugin project I'm working with where we check where users are storing various template/style files and enqueue accordingly.

Best of both worlds we enqueue going forward but maintain backpat and a happy user base, importantly it removes the necessity for child themes to have to override something which they may struggle with doing and those of us that do our own things with styles are also free to @import or enqueue as we see fit, again not having to firstly remove/cancel out behavior

@Boone do we care what users may or may not do? the matter of enqueueing styles is theme related, happening as it does in the themes functions file, that BP now does this == 'a good thing', that I may decide to fly against that best practise my own foolish lookout- and likely find myself damned for all eternity.

comment:33 karmatosed3 years ago

Throwing a slight request 'spanner' into works. Could we have also it part of a function so we can indeed enqueue even child themes. If have theme_support x then enqueue type of thing? I ask this as someone whose in midst of child themes with enqueue and know a few others are as word was out we should convert.

Quite frankly as a theme developer once you get head around the enqueue it's really a ton better than imports in my opinion.

If that's what is being suggested great but we really just need to only have the fall back (which I think is good option) for those themes that haven't been updated. I may also be confused a little and we're still proposing to keep enqueue even for children which is great.

My vote is enqueue with fall back as we have to stop the @import and go enqueue eventually this is something we should do for backpat.

Last edited 3 years ago by karmatosed (previous) (diff)

comment:34 hnla3 years ago

I thought the suggested idea was to kill the enqueue for child theme, which would leave the child theme needing to work out it's own approach, it could still go and enqueue those files but would need to do so itself which is fine isn't it?

One thing about this slight disparagement of @import, @import is a mechanism belonging to the CSS protocol/specs and the frontend browser to server, there is nothing empirically wrong with it and it serves a specific and fairly important role in the Cascade. Enqueueing is a WP proprietary mechanism designed chiefly to deal with loading of files when still in the mid tier api backend, it's benefits are unquestionable but most importantly revolve around the question of loading styles and scripts in an ordered fashion, one that can resolve dependencies and avoid loading clashes. So - although I may be contradicting myself - I still believe there's nothing wrong with loading the themes primary styles.css with @imports, and there is still the question I asked a while back what happens with what is now essentially a style.css file that only serves a purpose for the backend it now gets needlessly delivered to the browser? is that good?

comment:35 karmatosed3 years ago

Hmmm not having it work at all really isn't great for child themes as I for one have used that now in child themes and fairly sure others will have been doing that. However, if the function still exists hopefully it would work out. My point is what about child themes that use that function (with that name) now. I'm probably not the only theme designer whose just done that work in some themes :)

To me I like enqueue and don't like @import - why? Flexibility and child themes. It just makes far more sense for child themes than @import does in long run and removes a ton of updating issues. There are other reasons but to me that's a glaring one.

I think should we / shouldn't we is a bit to one side here though (valid debate but not the key one now). Ultimately we have to find some way to either educate / information or backpat - possibly both.

My vote would be a fall back if theme has no theme_support or function call to the enqueue function. I may be also in a few minutes asking for the moon on a stick and kitchen sink :)

comment:36 johnjamesjacoby3 years ago

Mixing @import with link'ed CSS is bad. The best explanation I have is:
http://www.stevesouders.com/blog/2009/04/09/dont-use-import/

Since WordPress plugins link to their own CSS, and WordPress has an API, we should use it.
I.E - If we can enqueue, we should.

From what I can tell, we are not queueing up an empty style.css anymore.

Let's be inventive if we need to. Can bp-default detect that it's the parent theme, and enqueue the child CSS for it? I think my suggestion above was a good compromise of fixing and backpat, too.

comment:37 boonebgorges3 years ago

jjj's comment gave me an idea. How about:

  • Before bp-default enqueues its style, if the current theme is a child of bp-default, we parse (with php) the child theme's stylesheet to look for an @import rule for the bp-default css
  • if we find a match, we bail, letting the @import go through - no enqueuing.

New themes will be encouraged to enqueue; old themes will continue to work a they used to. bp-default, used as a theme in itself, will enqueue.

I can work up a patch tomorrow, but I wanted to throw the idea up here first, to see if anyone else thought that it sounded like a decent compromise.

comment:38 hnla3 years ago

@jjj point taken however that article is a mixed bag, it gives a slightly misleading impression that @import is simply BAD! Concurrent connections, parallel downloads are a server configuration issue primarily and limits can and will be set possibly negating the benefits gained from simply linking to files - the real issue of page load time in a WP installation is dictated by the sheer number of linked files and scripts lining up to be downloaded, in truth having a few @imports in the mix is not noticeably going to slow things down - but! I do defer on this and do not want to drag things off topic and will actually re-visit my @imports and enqueue them to avoid my mix of enqueued styles and non enqueued.

comment:39 boonebgorges3 years ago

  • Keywords has-patch added; close removed

As I looked into the issue I think I came to understand it a bit better. 3389.02.patch is a suggested fix, more or less along the lines of one of jjj's suggestions.

Essentially, 3389.02.patch enqueues the child theme's stylesheet manually. There are a few additional niceties involved. I open the stylesheet to see if there is an @import rule for bp-default's default.css. (1) If YES, then I enqueue the child theme's stylesheet and block bp-default from enqueuing its own styles (thus saving an HTTP request, as default.css will only be fetched once). (2) If NO, then I enqueue the child theme's stylesheet, with a dependency on bp-default's default.css, and then allow bp-default to enqueue it's own style.

There is a bit of overhead involved in the fread() method, but I think it's better than attempting to load default.css twice over HTTP.

Here's how this pans out for different kinds of child themes:
(a) Existing child themes, and child themes in the future that use the @import method, will fall into category (1) above. Thus they will continue to work, unmodified. In this case, you lose the advantages of enqueuing, but you get the benefit that you don't break the whole theme :)
(b) Child themes that use the previously suggested method of overriding the pluggable function bp_dtheme_enqueue_styles() will continue to work as expected - they avoid this new logic altogether, because the function is never called.

With this fix, I suggest that we keep recommending child theme authors to use the bp_dtheme_enqueue_styles() override method for building their themes. It will eliminate the unnecessary overhead of using fread() on the child theme stylesheet, and it will take full advantage of WP's enqueuing architecture.

I like this solution, because it maintains 100% compatibility with old themes (at least as far as this enqueuing business is concerned ;) ) but ensures that the new enqueue method will get used as much as possible. (To be honest, I think that WP's core child theme infrastructure should do something along these lines.)

Feedback?

boonebgorges3 years ago

comment:40 boonebgorges3 years ago

r-a-y points out that file_get_contents() is faster. See 3389.03.patch.

boonebgorges3 years ago

comment:41 r-a-y3 years ago

The problem with this approach is this assumes that the child theme's stylesheet uses an @import rule.

This also doesn't solve the problem of child themes that have the stylesheet hardcoded in header.php.

I am revisiting the object buffering idea I had. Output buffer from 'get_header' to 'wp_head' at priority 0. Check to see if a stylesheet is hardcoded, if so use a version of Boone's patch to enqueue it while still in the 'wp_head' action at zero.

Since the 'wp_print_styles' hook hits wp_head at priority 8, this still gives us enough time to enqueue the stylesheet if necessary.

I'm going to test out my theory. If it works out well, I'll attach a patch ;)

Last edited 3 years ago by r-a-y (previous) (diff)

r-a-y3 years ago

comment:42 r-a-y3 years ago

3384.04.patch implements the ideas in my previous comment.

Except for the use of ob_start() in the 'get_header' action, I like this approach because:

  • We let older child themes continue to @import if they already set it up in their stylesheet.
  • We properly enqueue an older child theme's stylesheet, while stopping bp-default from being greedy by enqueueing default.css

I hate the fact that I'm using ob_start() though. Need some feedback for how to combat this when newer child themes decide to plug bp_dtheme_enqueue_styles().

Last edited 3 years ago by r-a-y (previous) (diff)

comment:43 follow-up: boonebgorges3 years ago

The problem with this approach is this assumes that the child theme's stylesheet uses an @import rule... This also doesn't solve the problem of child themes that have the stylesheet hardcoded in header.php.

If by the first sentence you mean the same thing that you say in the second sentence, then yes. If you mean that my solution doesn't account for people using @import, I don't understand - I have regex for that very purpose.

That said, your solution is more foolproof, because it does look for hardcoded links to the stylesheet. That seems to be the main advantage of using ob_start() rather than opening the files with file_get_contents().

If a newer theme plugins bp_dtheme_enqueue_styles(), then they will be responsible for registering bp-default's CSS. I think that's reasonable. Anyone who plugs the function will know what they're doing, or at least they should if they're doing the plugging.

comment:44 in reply to: ↑ 43 r-a-y3 years ago

Replying to boonebgorges:

The problem with this approach is this assumes that the child theme's stylesheet uses an @import rule... This also doesn't solve the problem of child themes that have the stylesheet hardcoded in header.php.

If by the first sentence you mean the same thing that you say in the second sentence, then yes. If you mean that my solution doesn't account for people using @import, I don't understand - I have regex for that very purpose.

I do mean the same thing! Sorry for the confusion!

---

If a newer theme plugins bp_dtheme_enqueue_styles(), then they will be responsible for registering bp-default's CSS. I think that's reasonable. Anyone who plugs the function will know what they're doing, or at least they should if they're doing the plugging.

Yeah, I believe you're right!

The child theme would have to remove the 'get_header' hook for ob_start() as well by either overriding bp_dtheme_setup() or using remove_action().

That being said, I'm not really fond of the idea of using ob_start() in a theme, ya know? If this approach is implemented, hopefully we can remove this in v1.6.

Might want to get JJJ and pgibbs' take on things.

comment:45 boonebgorges3 years ago

Given all the different ways theme authors might load their stylesheets (enqueuing, including, hooking to wp_head or wp_print_scripts and hardcoding, hardcoding into header.php, etc), I don't know of a way to be fully general *without* actually printing the head of the document, and I don't really know a way to do it without doing ob_start().

comment:46 johnjamesjacoby3 years ago

The output buffer approach needs some work, because right now even bp-default gets loaded in the buffer which isn't necessary. I also don't like the idea of the output buffer in two separate functions as anything can happen in between that could cause severe breakage (another clever plugin creating its own buffer, for example.)

I think the output buffer method might be TOO clever, although it is a neat idea.

comment:47 boonebgorges3 years ago

Here's another idea: Enqueue the child theme's stylesheet as dependent on bp-default-main, and then enqueue bp-default-main. See 3384.05.patch.

This provides full backpat with existing @include methods. The big downside is that, in cases where @include is used, we request default.css twice.

But: is this that big a deal? Going forward, we can recommend that themes either leave out the @import rule, or override bp_dtheme_enqueue_styles(), or both. When you do either of those things, the duplicate HTTP request disappears. Themes that are not updated will just be a little inefficent. (But even then, given the aggressive way in which browsers cache stylesheets, it seems pretty negligible.)

boonebgorges3 years ago

comment:48 johnjamesjacoby3 years ago

-- I like that idea the best so far.

comment:49 hnla3 years ago

Think that idea is probably going to be as good as it gets, and will serve purpose, this issue is one that needs to be put to bed now :)

comment:50 boonebgorges3 years ago

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

(In [5124]) Modifies bp_dtheme_enqueue_styles() so that stylesheets of bp-default child themes are enqueued as bp-default dependent. This should maximize backward compatibility with the @import method, while providing maximum flexibility through the use of wp_enqueue_style(). Fixes #3389. Props johnjamesjacoby for the concept

Note: See TracTickets for help on using tickets.