Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load parent styles when child theme is activated #638

Merged
merged 1 commit into from
Dec 3, 2014

Conversation

grappler
Copy link
Collaborator

@grappler grappler commented Nov 2, 2014

After #635 I had another look at how the styles should be loaded when a child theme has been activated.

I spoke to @justintadlock and he mentioned that Hybrid-Core has been doing this for some time without any issues.

Responsive switched to this method soon after I wrote an article on it last year and we have had no complaints.

The pros:

  • The child theme does not require an @import and it just works with a blank style.css file.
  • Not requiring the @import should bring some performance gains.

@mor10
Copy link
Contributor

mor10 commented Nov 2, 2014

This is problematic if the child theme author wants to serve up a new stylesheet without relying on parent theme stylesheets. I think the idea of the @import (and now enqueue methods) is to all the child theme author to make the decusion about what stylesheets to load.

@grappler
Copy link
Collaborator Author

grappler commented Nov 3, 2014

@mor10 You can easily remove the parent stylesheet with:

<?php
function _s_child_scripts() {
    wp_dequeue_style( '_s-parent-style' );
}
add_action( 'wp_enqueue_scripts', '_s_child_scripts', 11 );

The majority of people with want to keep the parent style. For the minority there is this piece of code.

@obenland
Copy link
Member

obenland commented Nov 3, 2014

The way _s currently enqueues stylesheets is perfectly child theme friendly. I don't think adding an arbitrary enqueue, for the off chance that _s might be used as a child theme would make things any easier. Especially not with a placeholder-handle.

@import declarations (while not optimal) make it easy for inexperienced theme developers to make their child theme work. And I think it is perfectly acceptable to have advanced child theme authors dequeue and enqueue scripts and styles as they please.

This is a starter theme, let's keep things light. ;)

@obenland obenland closed this Nov 3, 2014
@grappler
Copy link
Collaborator Author

grappler commented Nov 3, 2014

I don't think adding an arbitrary enqueue, for the off chance that _s might be used as a child theme would make things any easier.

This is not so that _s is able to be used as a child theme but so when creating a child theme for _s based themes a style.css can be created without using @import.

By including this code the only thing the user would need to create a style.css and paste this header information in. No fiddling with @import.

/*
 Theme Name:   _s Child
 Description:  _s Child Theme
 Template:     _s
 Version:      1.0.0
*/

@obenland
Copy link
Member

obenland commented Nov 3, 2014

Ah, that makes more sense.

@obenland obenland reopened this Nov 3, 2014
@samikeijonen
Copy link
Contributor

+1.9 for this one.

@justintadlock
Copy link

I just wanted to chime in for a moment since I had been discussing this with @grappler behind the scenes.

It's pretty well accepted that child themes are the preferred method of modifying themes. Therefore, users of themes forked from _s should be using a child theme to do their modifications.

I've been using the proposed method for quite some time in my own themes. The one positive thing I've noticed is that there's been a dramatic decrease in the number of "why doesn't the parent styles load?" type of questions from normal users. Many of them are familiar with the @import method. However, they often completely screw this up. Sometimes, they've changed the parent theme folder name. Sometimes, they simply have a typo. Using @import for this is ripe with problems because it's a static piece of code and requires regular users to write the code.

What this method does is promote good dev practice by not using @import. It takes away another entry to barrier in teaching users to use child themes. And, it makes for fewer support questions for theme authors.

It's much easier to teach a handful of users how to dequeue parent theme styles rather than explain to all users how to import them. Because _s is such a widely-used starter theme, the inclusion of this bit of code would promote good practices all around.

@obenland
Copy link
Member

obenland commented Nov 3, 2014

Yeah, you can disregard pretty much all of my first comment, I misinterpreted the patch.

stebrech added a commit to WPZOO/Penguin that referenced this pull request Nov 15, 2014
@philiparthurmoore
Copy link
Collaborator

@justintadlock says:

Because _s is such a widely-used starter theme, the inclusion of this bit of code would promote good practices all around.

Yes, I agree completely.

The only question I have for @grappler is why didn't you also add the RTL bits to this pull request like you did here?

@grappler
Copy link
Collaborator Author

grappler commented Dec 2, 2014

@philiparthurmoore The reason was that the initial discussion did not include the RTL stylesheet and I thought the PR would have a better chance if it only changed one thing and not two.

@philiparthurmoore
Copy link
Collaborator

@grappler Thanks for the explanation. On the one hand I feel like since _s has an RTL file we need to add those RTL bits into this pull request, and on the other hand I prefer using .rtl in the primary stylesheet and wouldn't have a need for this is_rtl addition into the theme. Because we do not know that people will create RTL styles for their themes, and because I'd rather not risk there being not found errors for the parent's RTL sheet, let's go ahead and leave that out. Your PR as it stands now is a good one. I'll test it and merge.

@fklein-lu
Copy link
Contributor

I disagree with merging this.

This is a support issue, not a technical one. If user's are able to grok @import, then they will be able to understand a code snippet you provide to enqueue the parent stylesheet correctly.

Parent themes should be concerned with parent theme styles and child themes with child theme styles. Having is_child_theme() in a parent theme is code smell to me and it should be avoided.

Especially because this code is making assumptions on the part of the child theme developer. I don't see why you should have to dequeue styles you didn't want to have in your child theme in the first place.

This change should be reverted until there is a consensus that this is in fact a good idea to include in _s.

@philiparthurmoore
Copy link
Collaborator

I disagree with merging this.

Lol. You're killing me @fklein-lu! Where you been?!

This is a support issue, not a technical one. If user's are able to grok @import, then they will be able to understand a code snippet you provide to enqueue the parent stylesheet correctly.

Very valid point but isn't there some credence to the @import performance issue? I don't really see this as a support issue at all. If I'm developing a theme with the intention of making a suite of child themes for it then having this in is fantastic, IMO.

Especially because this code is making assumptions on the part of the child theme developer. I don't see why you should have to dequeue styles you didn't want to have in your child theme in the first place.

Are you referring to cases when you make a child theme that does not want to use a parent's stylesheet? This is very valid.

This change should be reverted until there is a consensus that this is in fact a good idea to include in _s.

Consensus around _s is such a vague concept. I'm putting in a sprint this week to clean and close things as I see fit. I'm 1000% happy to revert this if you want to open another Issue for further discussion? Wish we had this input before I dove into this.

The great thing about _s is that we can change this as much as we want. Nothing is ever set in stone. Merging and then reverting and then merging again won't break the world. 😄

@philiparthurmoore
Copy link
Collaborator

The only valid reason I can see not to have this in is for cases when someone wants to make a child theme and not use a parent's styles. Given how much self-hosted support I've had to do over the last many many years I see this as such a very big edge case.

Anyone else want to chime in on this? In general I think that this is a really good approach to developing publicly released themes that may be used as parents (all themes, basically); I'm not sure that I'm convinced this is bad in any way but, as always, input is really appreciated.

@philiparthurmoore
Copy link
Collaborator

As much as I hate the @import stuff I'm going to revert this for a bit to get more input.

Side note: Issues like this really shouldn't be sitting around for a month with little input. It's not something that should take this long to chat about.

@fklein-lu
Copy link
Contributor

Lol. You're killing me @fklein-lu! Where you been?!

I'm sorry that I missed this, I saw the merge notification and then saw the related discussion.

I agree with you that we can always revert and and then later commit again. I really appreciate you putting all the work into _s.

I just consider that when there is little feedback concerning a ticket, it's better to ping people and ask for feedback rather than merging the PR. Because in this case, not participating in the discussion does not necessarily mean agreeing.

The only valid reason I can see not to have this in is for cases when someone wants to make a child theme and not use a parent's styles. Given how much self-hosted support I've had to do over the last many many years I see this as such a very big edge case.

I've mentioned other concerns in my comment, and I don't think we should brush those over.

Considering .org support, I don't think that child themes are still as important as they used to be. I dare to say that most users only want to add CSS customizations, for which there are plugins available. Theme developers should guide their users towards those instead of taking the route of a child theme.

@samikeijonen
Copy link
Contributor

+1 for this pull request.

@obenland
Copy link
Member

obenland commented Dec 3, 2014

@fklein-lu

I just consider that when there is little feedback concerning a ticket, it's better to ping people and ask for feedback rather than merging the PR. Because in this case, not participating in the discussion does not necessarily mean agreeing.

That's an interesting assumption. Why can't we assume that people with an opinion voice that opinion in the ticket?

Parent themes should be concerned with parent theme styles and child themes with child theme styles. Having is_child_theme() in a parent theme is code smell to me and it should be avoided.

There is plenty of things themes do to be child theme friendly. Including using get_stylesheet_uri() rather than get_template_directory() . '/style.css'. Developing in a child theme friendly way is even a WPTRT guideline. ;)

I agree with @philiparthurmoore, it's good approach for any publicly released theme. I'd also consider child themes that do not use their parent's CSS at all, an edge case.

@fklein-lu
Copy link
Contributor

That's an interesting assumption. Why can't we assume that people with an opinion voice that opinion in the ticket?

Because it's pretty easy to miss things, especially for somebody at Automattic. If a couple of people have given an opinion and the ticket has traction, then it's good to go. But this one was having little activity and suddenly got merged, I'd appreciate it more if we would not just push things in too fast.

As far as the WPTRT is concerned, I don't think that being child theme friendly is still a guideline. But when considering child themes, we could do other things to make _s more child theme friendly, but I think the negatives outweigh the benefits in these cases, that's why it they are not in _s.

I'm fine with this PR going in if nobody else has any arguments against it. I can follow my own advice and remove it in the themes I write. :)

@mor10
Copy link
Contributor

mor10 commented Dec 3, 2014

This is really a question of usability from the end-user perspective. The new recommended method for enqueueing parent theme stylehseets in child themes from the Codex and Handbook puts an unnecessary level of complexity at the hands of the end-user who more than likely just wants to use a child theme to change some colors or sizes. The proposed solution here fixes this problem in a clean and concise way and makes child theme authoring easier for themes built on _s. I think this merge is not only warranted but necessary to ensure consistency for the people who use themes and want to build child themes off them.

I do have one request though: As I've stiplulated before, this method only works if there is only one stylesheet in the parent theme, but _s sets the stage for a situation where there are several stylesheets and layouts are broken out into their own files. To ensure consistency it would be ideal if _s either shipped with the framework in place to ensure layout stylesheets are loaded properly both in the theme and in child themes or that documentation is provided to make it clear to developers how to do this in a uniform way.

The method @grappler made for my theme Simone works really well.

@obenland
Copy link
Member

obenland commented Dec 3, 2014

The new recommended method for enqueueing parent theme stylehseets in child themes from the Codex and Handbook puts an unnecessary level of complexity at the hands of the end-user who more than likely just wants to use a child theme to change some colors or sizes. The proposed solution here fixes this problem in a clean and concise way and makes child theme authoring easier for themes built on _s

I agree with most of that. s/end-user/theme author.

[…] it would be ideal if _s either shipped with the framework in place to ensure layout stylesheets are loaded properly both in the theme and in child themes […]

It does. It's called wp_enqueue_style() and all theme authors have to do is to define dependencies. :)

@mor10
Copy link
Contributor

mor10 commented Dec 3, 2014

Read "framework" as "example". Dependencies are only going to work here if
it's done in a very specific way. An example would ensure consistency.
Without an example you'll get every random solution under the sun. We want
consistency, and _s is a great place to set that standard.

On Wed, Dec 3, 2014, 2:27 PM Konstantin Obenland notifications@github.com
wrote:

The new recommended method for enqueueing parent theme stylehseets in
child themes from the Codex and Handbook puts an unnecessary level of
complexity at the hands of the end-user who more than likely just wants to
use a child theme to change some colors or sizes. The proposed solution
here fixes this problem in a clean and concise way and makes child theme
authoring easier for themes built on _s

I agree with most of that. s/end-user/theme author.

[…] it would be ideal if _s either shipped with the framework in place to
ensure layout stylesheets are loaded properly both in the theme and in
child themes […]

It does. It's called wp_enqueue_style() and all theme authors have to do
is to define dependencies. :)


Reply to this email directly or view it on GitHub
#638 (comment).

@fklein-lu
Copy link
Contributor

If you have different stylesheets, wouldn't the best approach be to use get_template_directory_uri()? That way they are loaded from the parent theme when a child theme is active and overriding styles can be done the child theme's style.css.

gatespace added a commit to gatespace/_s that referenced this pull request Dec 8, 2014
* upstream/master:
  Revert Automattic#638 for a bit.
  Update POT. Fixes Automattic#645.
  Load parent styles when child theme is activated

Conflicts:
	languages/_s.pot
@grappler
Copy link
Collaborator Author

My response to some of the critic.

This is a support issue, not a technical one. If user's are able to grok @import, then they will be able to understand a code snippet you provide to enqueue the parent stylesheet correctly.

They will also be able to understand a code snippet explaining how to dequeue a stylesheet too, so I don't see the issue here. To get the correct order of the stylesheet in the child theme is a bit more complicated when using wp_enqueue_style()

I don't see why you should have to dequeue styles you didn't want to have in your child theme in the first place.

Because more people will want to have the parent styles than those who do not.

Considering .org support, I don't think that child themes are still as important as they used to be. I dare to say that most users only want to add CSS customizations, for which there are plugins available.

The usefulness of child themes is being able to apply the changes to multiple sites with having to copy and paste.

This solution has been implemented in a number of popular theme with no issues and no increase in support requests.


After thinking about how this solution is not the best way. My new solution would be this commit grappler@3fd84b4.

I have create a core ticket to add a new function to get the parent stylesheet URI. https://core.trac.wordpress.org/ticket/30797

I would love some feedback on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants