Twenty Fifteen: Theme Review: Stylesheets and Scripts

[emiluzelac]

Quick heads up guys:

• No hard coding of scripts, styles and Favicons unless a browser workaround script. Everything should be enqueued.

<script>(function(){document.documentElement.className='js'})();</script>

Please ​see

The workaround noted above refers to e.g.

<script src="<?php echo esc_url( get_template_directory_uri() ); ?>/js/html5.js"></script>

Yep, it's small, but it still needs to be moved and added via functions.php :)

Thanks for the great theme!

[iamtakashi]

Replying to emiluzelac:

• No hard coding of scripts, styles and Favicons unless a browser workaround script. Everything should be enqueued.

<script>(function(){document.documentElement.className='js'})();</script>

See this discussion about this. #30056

The workaround noted above refers to e.g.

<script src="<?php echo esc_url( get_template_directory_uri() ); ?>/js/html5.js"></script>

See this ticket. #16024

How do you approach to these issues?

[philiparthurmoore]

I'm not convinced that moving this into an enqueue is better. Better for guidelines? Maybe. But the flashing is very annoying and very real. What reason other than "doesn't meet theme review guidelines" is there to make this change?

[iamtakashi]

Replying to philiparthurmoore:

I'm not convinced that moving this into an enqueue is better. Better for guidelines? Maybe. But the flashing is very annoying and very real. What reason other than "doesn't meet theme review guidelines" is there to make this change?

I feel the same way. I don't see a good reason to not making these exceptions until we have a good solution for each issue.

[ocean90]

That's all fine as it is. See also the linked tickets in comment:2.

[emiluzelac]

Help me understand please: with this in header, theme does not really meet the guidelines, how is that not good reason?

[philiparthurmoore]

Replying to emiluzelac:

Help me understand please: with this in header, theme does not really meet the guidelines, how is that not good reason?

Another way to put it would be that if there were no guidelines currently in place what would be the best way about this? I think that Twenty Fifteen in its current state does this correctly.

[emiluzelac]

Not really sure what is the best way, you can see: ​https://themes.trac.wordpress.org/ticket/20079#comment:3 for example.

This is not a question how to improve or do this better, guidelines should apply to all themes equally and if it does not, it will came back on us the reviewers :)

[cais]

Replying to emiluzelac:

Not really sure what is the best way, you can see: ​https://themes.trac.wordpress.org/ticket/20079#comment:3 for example.

This is not a question how to improve or do this better, guidelines should apply to all themes equally and if it does not, it will came back on us the reviewers :)

I would have to agree with this, especially with the reference to another "Automattic" theme using the same approach being happy to make the changes to the code that reflects current theme review team requirements and recommendations.

It is also very important that bundled themes follow these requirements and recommendations as many theme developers, new and old, will often use the bundled theme's code as a basis for their own works applying the same methods and constructs with the belief, "If Twenty X uses it, then it must be perfectly fine for me to use it too"

I can accept (although not like at all) a bundled theme being pushed through the queues ahead of all others (it is a WordPress repository after all) but I also believe it is still vitally important for the theme review team and the bundled themes authors to work together to find the best go forward approach; and, ignoring a guideline because you don't think its appropriate is not the best way of doing it.

Exceptions are fine, and there will always be considerations for them ... but wouldn't it be better to discuss them before rather than after the fact?

[philiparthurmoore]

Replying to emiluzelac:

Not really sure what is the best way, you can see: ​https://themes.trac.wordpress.org/ticket/20079#comment:3 for example.

This is not a question how to improve or do this better, guidelines should apply to all themes equally and if it does not, it will came back on us the reviewers :)

My apologies. I thought this was a code discussion, not a theme review discussion. (Seriously, no sarcasm intended.) I have no idea what the submission process for default themes is like so I cannot address that. My points above were specifically about the code.

If this is causing an issue with review guidelines, why not change them? What's wrong with adjusting the guidelines? Because it was done one way 4 months ago doesn't mean that it has to be done that way now. Jankiness and flashing does absolutely occur when the JS detection happens too late; shouldn't we be trying to address that performance issue rather than the issue of whether or not a theme is in line with review guidelines?

[iamtakashi]

We just need to change the guideline then.

It's because If we think about performance of WordPress themes, I believe how Twenty Fifteen does is the best way to address these issues.

But if somebody has better solutions that meet the current guideline, let us know. Otherwise, we just need to change the guideline.

[emiluzelac]

Do you guys have any idea what TRT even does and how much we do to make this work for all?

We can't just change the guideline because of one theme, that is not what team does.

Imagine we adjust the guidelines to meet the theme codes, where would that lead us to?

[iamtakashi]

As Drew pointed in Slack, these would fall into the category of "workaround", so we might not need to adjust the guideline after all.

I've suggested to change the guideline because I strongly believe the guideline should make WordPress make better, not sacrifice performance and compatibility to old browsers.

[greenshady]

I think some people may be misunderstanding what TRT is asking. I'll explain it in terms of code.

This line placed in header.php:

<script>(function(){document.documentElement.className='js'})();</script>

Should be moved to a callback hooked to wp_head(). I'd actually argue both should be moved. I'm not sure how that would mess up anything with the theme. It doesn't have anything to do with enqueueing the script.

One of the big reasons for this particular guideline to me is that child themes (or plugins) can't remove the JS for whatever reason without overwriting the entire header.php.

I do recognize that JS should be added when it's needed, not necessarily enqueued. Hardcoding JS right in the middle of a page is sometimes valid. But, for things that are being output directly into the header or the footer, we should try to stick to the core header and footer hooks.

[emiluzelac]

@iamtakashi guidelines accepts only one script in the header.php and that is HTML5

Rough example to make this work: (disregard the priority)

function twentyfifteen_js_class () {
    echo '<script>(function(){document.documentElement.className='js'})();</script>'. "\n";
}
add_action( 'wp_head', twentyfifteen_js_class', 1 );

[iamtakashi]

@emiluzelac, Thanks for the clarification about html5.js.

Also thanks for your suggestion about another. Could you make a patch with your suggestion so that it's easy for everyone to test the performance?

[emiluzelac]

@iamtakashi You got me, I've never created or submitted a patch before :( I can try.

[emiluzelac]

Can I submit a patch over: ​https://github.com/WordPress/WordPress/ ?

[iamtakashi]

@emiluzelac, no worries! You can find more info about creating and submitting patches here:

​https://make.wordpress.org/core/handbook/working-with-patches/patches-with-command-line/
​https://make.wordpress.org/core/handbook/working-with-trac/submitting-a-patch/

As the first link states, make sure you create a patch from the root. This makes a lot easier for people to test.

Patches should be created from the root directory of your WordPress SVN install.

[obenland]

Replying to greenshady:

This line placed in header.php:

<script>(function(){document.documentElement.className='js'})();</script>

Should be moved to a callback hooked to wp_head().

I think that's reasonable. It will not change the way it works now and child themes will have it easier to modify it, as Justin mentioned.

[chipbennett]

I'll have a patch submitted this evening, if someone doesn't beat me to it.

[emiluzelac]

Chip, I was thinking that this would be good for me, but no worries, as long as we get things going. We can wait till this evening for sure :)

[chipbennett]

Moves js flash fix script to a callback, enqueued at wp_head, priority 0. Output should render exactly as current <head> output in header.php.

[philiparthurmoore]

Chip, this is very minor but what are your thoughts on changing Enqueue script to fix JS flash to something more along the lines of JavaScript Feature Detection or JavaScript Detection or something a bit more general? I'm not married to this at all; would love your thoughts.

[chipbennett]

I'm open to any wordsmithing deemed appropriate. I'm likewise not married to the phpDoc text, callback name, etc.

[iamtakashi]

Thanks for the patch, Chip.

Like mentioned, the naming and PHPDoc could be better but the patch is working well in my testing. And a patch should be generated from the root :)

[philiparthurmoore]

I have absolutely no idea what I was going on about two weeks ago. I just ran a bunch of no JS tests with a different theme that I'm working on and using wp_enqueue_scripts was totally fine. The only real difference between wp_head and wp_enqueue_scripts was that the no JS detection came under the page title when wp_enqueue_scripts was used.

Chip's intent was good I think, given the discussion, but I tend to think that we could even use wp_enqueue_scripts with Chip's approach just fine. So this:

add_action( 'wp_head', 'twentyfifteen_enqueue_js_fix', 0 );

Becomes this:

add_action( 'wp_enqueue_scripts', 'twentyfifteen_enqueue_js_fix' );

If I could go back in time I'd probably kick myself, but here we are. Sorry for the stubbornness earlier.

[valendesigns]

Refreshed the patch to be created from the root. Also did a little tidying up, renaming, and added additional docs. I know some of you don't want to move this out of the header, but I don't see why when it's location in the DOM is not changing. What is the downside here?

[philiparthurmoore]

Replying to valendesigns:

Refreshed the patch to be created from the root. Also did a little tidying up, renaming, and added additional docs. I know some of you don't want to move this out of the header, but I don't see why when it's location in the DOM is not changing. What is the downside here?

This definitely needs to be in the header, not the footer. The entire issue here is to prevent flashing of a non-JS menu before the JS detection kicks in.

[valendesigns]

Replying to philiparthurmoore:

This definitely needs to be in the header, not the footer. The entire issue here is to prevent flashing of a non-JS menu before the JS detection kicks in.

Sorry if you misunderstood me, but I didn't mention anything about the footer. I'm saying I don't see the problem with moving into a filter if it's stays in the exact same place in the DOM. Meets the theme guidelines and works as expected.

[philiparthurmoore]

Replying to valendesigns:

Replying to philiparthurmoore:

This definitely needs to be in the header, not the footer. The entire issue here is to prevent flashing of a non-JS menu before the JS detection kicks in.

Sorry if you misunderstood me, but I didn't mention anything about the footer. I'm saying I don't see the problem with moving into a filter if it's stays in the exact same place in the DOM. Meets the theme guidelines and works as expected.

"I know some of you don't want to move this out of the header..."

I read that wrong. :)

[valendesigns]

No worries. I see the confusion, should have wrote header.php instead.

Cheers,
Derek

[peterwilsoncc]

Append JS class to HTML element

[peterwilsoncc]

In 30770.1.diff, I've modified @valendesigns' code to append a js class to the HTML element rather than replace all classes on the HTML element.

This allows child themes to add classes to the HTML element either directly or by using modernizr or similar.

[iamtakashi]

Replying to peterwilsoncc:

We do need to replace no-js class with the current CSS:

.no-js .main-navigation ul ul {
	display: block;
}

[peterwilsoncc]

Replying to iamtakashi:

We do need to replace no-js class with the current CSS:

Uploaded 30770.2.diff with a modified version of the Paul Irish technique.

[valendesigns]

Replying to peterwilsoncc:

Replying to iamtakashi:

We do need to replace no-js class with the current CSS:

Uploaded 30770.2.diff with a modified version of the Paul Irish technique.

Thank you for adding an updated patch. However, I think 30770.diff​ is still the correct course of action here. The code is being loaded before all other scripts and in the case of child themes or modernizr they would still be able to add classes to the html element by overriding the modification in a separate script, since they are called after wp_head via the wp_enqueue_scripts action. Please correct me if I'm wrong, but I don't see a reason to change the JavaScript to make it any more compatible. The issue here (IMO) is how this particular piece of code is being loaded into header.php and not how it changes no-js to js in the html element.

[peterwilsoncc]

Replying to valendesigns:

Thank you for adding an updated patch. However, I think 30770.diff​ is still the correct course of action here. The code is being loaded before all other scripts and in the case of child themes or modernizr they would still be able to add classes to the html element by overriding the modification in a separate script, since they are called after wp_head via the wp_enqueue_scripts action. Please correct me if I'm wrong, but I don't see a reason to change the JavaScript to make it any more compatible. The issue here (IMO) is how this particular piece of code is being loaded into header.php and not how it changes no-js to js in the html element.

That's correct, any enqueued javascript runs after the code inserted into wp_head

My aim is to protect against two things:

• child themers adding a class to the HTML element directly into a header.php override.
• child themers doing_it_wrong by hard coding modernizr or similar into a header.php override.

The former being more important than the latter.

In terms of opinionated coding, the purpose is to replace no-js with js so that's what I think it should do.

Anyway, that was my poorly explained logic - I'll bow out now.

[valendesigns]

Replying to peterwilsoncc:

My aim is to protect against two things:

• child themers adding a class to the HTML element directly into a header.php override.
• child themers doing_it_wrong by hard coding modernizr or similar into a header.php override.

If we're trying to fix those use cases in this ticket, I think the JavaScript should be changed to the code below. I've added a new patch.

document.documentElement.className = document.documentElement.className.replace('no-js','js')

[peterwilsoncc]

That works too.

You will need to use the regexp /\bno-js\b/ to ensure it only replaces the no-js class. ATM you'll catch anything containing the sub-string, say casino-jschiffer.

[valendesigns]

Added a refreshed patch.

[SergeyBiryukov]

30770.4.diff​ looks good to me.

[iamtakashi]

I've tested 30770.4.diff. The flash doesn't occur and an additional CSS class to the html tag in child themes stays intact.

[lancewillett]

In 31184:

Twenty Fifteen: move js and no-js class name functionality out of header template and into a wp_head hook in functions.php file so that the JavaScript functionality isn't hard-coded into a template file.

Fixes #30770, props chipbennett, valendesigns, and peterwilsoncc.