WordPress.org
Get WordPress

Make WordPress Core

Opened 15 years ago

Closed 15 years ago

#12932 closed enhancement (fixed)

Use the enqueue API for twentyten

Reported by: ptahdunbar's profile ptahdunbar Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Themes Keywords: has-patch needs-testing
Focuses: Cc:

Description

Patch removes the hardcoded stylesheet link and comment reply js from the the twentyten <head> tag and replaces them with their standard wp_enqueue_script/style calls.

Attachments (2)

enqueue_twentyten.diff​ (2.1 KB) - added by ptahdunbar 15 years ago.
thread_comments_check.diff​ (694 bytes) - added by ptahdunbar 15 years ago.
adds a check for thread_comments so the js doesn't load when it's not needed

Download all attachments as: .zip

Change History (12)

@ptahdunbar
15 years ago

#1 follow-up: @freddyware
15 years ago

  • Type changed from defect (bug) to enhancement

Isn't it pretty pointless to remove a hardcoded stylesheet in a template? After all, themes are supposed to be associated with a stylesheet... unless this modification is supposed to make it possible to remove the stylesheet from a theme? (Why would someone do that?)

To date, I have not yet seen a single theme that uses enqueue to load their stylesheets. It would make theme development (e.g. in a testing / IDE environment) a LOT harder.

Moreover, this isn't really a bug; it's an enhancement.

#2 in reply to: ↑ 1 ; follow-up: @mikeschinkel
15 years ago

Replying to freddyware:

Isn't it pretty pointless to remove a hardcoded stylesheet in a template? After all, themes are supposed to be associated with a stylesheet... unless this modification is supposed to make it possible to remove the stylesheet from a theme? (Why would someone do that?)
To date, I have not yet seen a single theme that uses enqueue to load their stylesheets. It would make theme development (e.g. in a testing / IDE environment) a LOT harder.
Moreover, this isn't really a bug; it's an enhancement.

I'm not 100% sure of this, but I'm pretty sure that not using enqueue should be considered a bad practice, even if it is a bit harder. Why? If not used then WordPress can't collect all the stylesheets and serve them on one HTTP GET request. Not using enqueue thus dooms the website to many unnecessary HTTP requests and violates the first "rule" in website performance improvement, see:

​http://developer.yahoo.com/performance/rules.html

JMTCW, anyway.

#3 in reply to: ↑ 2 ; follow-up: @freddyware
15 years ago

  • Component changed from Template to Themes

Replying to mikeschinkel:

Replying to freddyware:

Isn't it pretty pointless to remove a hardcoded stylesheet in a template? After all, themes are supposed to be associated with a stylesheet... unless this modification is supposed to make it possible to remove the stylesheet from a theme? (Why would someone do that?)
To date, I have not yet seen a single theme that uses enqueue to load their stylesheets. It would make theme development (e.g. in a testing / IDE environment) a LOT harder.
Moreover, this isn't really a bug; it's an enhancement.

You bring up a very good point! Plugins like W3 Total Cache already help with minifying JavaScript and CSS, although I'm not sure whether W3 Total Cache does so with regular expressions on the HTML page (replacing <script> and <link> tags) or by tapping into enqueue. Certainly using enqueue will make this task of minifying easier.

However, I still stand by the argument that, for a theme developer, it is much much harder to debug and test a theme with an enqueued stylesheet than with a <link href> tag that IDEs and editors recognize.

Also, this is technically a theme issue. Let's see if this ticket will get more attention there.

#4 in reply to: ↑ 3 ; follow-up: @mikeschinkel
15 years ago

Replying to freddyware:

However, I still stand by the argument that, for a theme developer, it is much much harder to debug and test a theme with an enqueued stylesheet than with a <link href> tag that IDEs and editors recognize.

I assume you mean it's harder to debug because it's hard to view your CSS source with the correct line numbers and file names if they are concatenated, correct? If that's the concern I agree that it would make it harder to debug. However I also would argue that your criticism is wrongly placed.

I'm unfortunately not that familiar with the enqueue and thus can't explain how you bypass concatenation during development but I assume you must be able to by defining a constant or some other method. OTOH, if bypassing concatenation is not currently possible than I'd suggest rather than argue against enqueuing that we instead argue for ability to bypass concatenation when a define is set, i.e. maybe WP_DEBUG_STYLES.

Maybe someone like Ptah who better knows how enqueuing works can clarify this issue?

#5 @freddyware
15 years ago

  • Keywords needs-testing added

Hm. I looked at how the stylesheet is currently loaded, and it DOES use a dynamic PHP method to get the CSS location. If this is the case, the ticket should indeed improve the handling of the stylesheet.

My previous criticism was related to the fact that the following

<link rel="stylesheet" type="text/css" media="all" href="/wp-content/themes/theme-name/style.css" />

could be used by a WYSIWYG editor or IDE with preview features to test layout changes. However, I see that a developer could just modify their theme in conjunction with a development installation of WordPress.

+1 for this ticket.

#6 in reply to: ↑ 4 @duck_
15 years ago

Replying to mikeschinkel:

I'm unfortunately not that familiar with the enqueue and thus can't explain how you bypass concatenation during development but I assume you must be able to by defining a constant or some other method. OTOH, if bypassing concatenation is not currently possible than I'd suggest rather than argue against enqueuing that we instead argue for ability to bypass concatenation when a define is set, i.e. maybe WP_DEBUG_STYLES.

define( 'SCRIPT_DEBUG', true ); in wp-config.php

@ptahdunbar
15 years ago

adds a check for thread_comments so the js doesn't load when it's not needed

#7 @nacin
15 years ago

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

(In [14339]) Check thread_comments before loading the comment-reply script. props ptahdunbar, fixes #12932

#8 @nacin
15 years ago

After talking about this with Ptah and Viper007Bond, we're going to leave style.css as is. It's just unnecessary cycles for something that should be ubiquitous in the theme. Likewise, the theme calling comment-reply as an enqueue is fine, if not better.

I committed a patch that checks whether we actually need comment-reply before enqueueing it.

#9 @zeo
15 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Why only add check thread_comments in [14339]?

How about this conditions:

  • comments_open()
  • pings_open()
  • is_user_logged_in()
  • comment_registration()
  • get_comment_number() == 0

Hehehe.

#10 @nacin
15 years ago

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

The rest of those checks generate queries. I don't see the need for that. thread_comments is an autoloaded option, so no additional weight. We've been doing only is_singular() checks for quite a while (themes too), so this is a simple improvement with no decreased performance in any regard.

Note: See TracTickets for help on using tickets.