WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#24595 closed enhancement (fixed)

Twenty Thirteen: Load Genericons in a More Plugin-friendly Way

Reported by: celloexpressions Owned by: lancewillett
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: has-patch commit
Focuses: Cc:

Description

Genericons are a useful tool for plugins, but it is currently difficult to use them in a plugin in an efficient way. It would be nice if Twenty Thirteen loaded them in a way that would both play nicely with plugins that want to use them and set a standard for how plugins should load them.

I came up with a couple of options:

  • Package Genericons directly with WordPress, instead of bundled through Twenty Thirteen, and register as a style in core (wp_register_style()), then wp_enqueue_style('genericons') in both Twenty Thirteen and plugins. Plugins could also package Genericons if targeting WordPress <3.6. The loaded stylesheet would just contain the @font-face declaration. But having Genericons available directly in core wouldn't be as appropriate as, for example, having Dashicons available (for front-end use) would be.
  • Move the @font-face declaration to its own file and load it with wp_register/enqueue_style(). Plugins could check for Genericons with wp_style_is() and load them if necessary, which would decrease the potential to load from multiple places. This would establish a "correct" method for loading Genericons in plugins and at least provide the tools to avoid duplicate declarations, but could be messier to implement in plugins.

Both methods have advantages and disadvantages, and there are probably additional possibilities, but I think we should do something a bit more standardized than the current setup (loading Genericons from the main style.css file).

Attachments (5)

24595.patch (128.8 KB) - added by obenland 2 years ago.
24595.1.patch (128.8 KB) - added by obenland 2 years ago.
24595.2.diff (24.1 KB) - added by obenland 2 years ago.
24595.2.patch (1.1 KB) - added by DrewAPicture 2 years ago.
Load genericons as an editor stylesheet
24595.3.patch (1.1 KB) - added by DrewAPicture 2 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 follow-up: @Ipstenu2 years ago

I don't think Genericons should be in core (and I'm not just saying that as the author of a plugin that brings 'em in). Dashicons makes sense, it's actually a part of WordPress Core. Genericons, and Font-Awesome, and all other fonts for that matter that are used on the front end of WP are subjective and don't belong in core. But... Realistically this is the same struggle anyone faces when a theme includes a font that a plugin does as well, and perhaps a better way to look at this is more ... generically.

"How can we make it easier to include fonts (be they font icons or regular ones) in a way that a plugin and a theme that both include it don't stop all over each other?"

Now if TwentyThirteen 'called' genericons instead of bundling all the CSS inline in wp-content/themes/twentythirteen/style.css, then yeah, that'd be easier :) But it only works as far as I know someone else is using the code. If devs aren't consistent in the naming, we hit the same problem problem faced with jquery files that aren't in code that people pull in via plugins. Even if we use wp_enqueue_style('genericons') there's no promise that Dudley Do-Right won't use wp_enqueue_style('dudley-genericons') instead, thus double loading again.

FWIW, I ran the Genericon'd plugin on TwentyThirteen for about a month without any conflicts, though perhaps not the best speed ;)

comment:2 in reply to: ↑ 1 @celloexpressions2 years ago

Replying to Ipstenu:

I don't think Genericons should be in core

I agree, Dashicons are make sense but Genericons are a theme/plugin component.

Realistically this is the same struggle anyone faces when a theme includes a font that a plugin does as well, and perhaps a better way to look at this is more ... generically.

At first I was considering proposing a wp_enqueue_fonts() feature, but then I realized that it would be loading css files anyway and figured it would be bloat. But maybe some sort of extension of wp_enqueue_style()?

Now if TwentyThirteen 'called' genericons instead of bundling all the CSS inline in wp-content/themes/twentythirteen/style.css, then yeah, that'd be easier :) But it only works as far as I know someone else is using the code. If devs aren't consistent in the naming, we hit the same problem problem faced with jquery files that aren't in code that people pull in via plugins. Even if we use wp_enqueue_style('genericons') there's no promise that Dudley Do-Right won't use wp_enqueue_style('dudley-genericons') instead, thus double loading again.

Obviously we can't be sure everyone will use the "right way" but it would be nice if there is a "right way" established by Twenty Thirteen so that plugins have the ability to play nicely if they want to. I had used 'genericons', but then noticed that you use 'genericons-styles' and include all of the other css in there. In my case Genericons are one of several options and having a separate place for them makes more sense.

Regardless, I think the best short-term solution is to load Genericons via wp_enqueue_style('genericons') in Twenty Thirteen so that plugins can play nicely, then in the longer term maybe look more broadly at font-loading options.

@obenland2 years ago

comment:3 @obenland2 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.6
  • Version set to trunk

Patch also updates Genericons to its latest version 2.09.

comment:4 @Jayjdk2 years ago

The attribute selector ([class*="genericon"]) is not working in IE6 so the /* IE7 and IE6 fixes. */ comment should probably be changed to /* IE7 fixes. */

Version 0, edited 2 years ago by Jayjdk (next)

@obenland2 years ago

comment:5 @lancewillett2 years ago

Even better would be if Genericons (or a trademark-free subset) were on Google Webfonts. We could just load from there, and not have to worry about committing updates to it in the future.

comment:6 @lancewillett2 years ago

  • Keywords commit added

comment:7 @lancewillett2 years ago

Hmm, I'm having trouble applying the latest patch.

patching file wp-content/themes/twentythirteen/fonts/genericons-regular-webfont.ttf
patch: **** malformed patch at line 11: @@ -49,7 +50,10 @@

comment:8 @lancewillett2 years ago

@obenland Can you try making the patch without the changed font files? I can pull them directly from Genericons.com instead.

@obenland2 years ago

comment:9 follow-up: @lancewillett2 years ago

A few questions:

  • Should we use "twentythirteen-genericons" as the namespace for the style enqueue? Plays nicer with plugins, probably.
  • Why did you go with the base64 code?
  • Why is the "Individual icons." and the rest of the CSS needed? This file should just be the exact code removed from style.css, I think.

comment:10 in reply to: ↑ 9 @obenland2 years ago

Replying to lancewillett:

  • Should we use "twentythirteen-genericons" as the namespace for the style enqueue? Plays nicer with plugins, probably.

I used 'genericons' in an effort to avoid loading the font multiple times in different namespaces.


  • Why did you go with the base64 code?
  • Why is the "Individual icons." and the rest of the CSS needed? This file should just be the exact code removed from style.css, I think.

I basically downloaded the Genericon 2.09 package from genericons.com and added some formatting. Joen will use that in its next update. The purpose was to make future maintenance as easy as dropping the new files in the fonts folder.

The base64 encoding was part of the 2.09 package.

comment:11 @lancewillett2 years ago

In 24515:

Twenty Thirteen: update Genericons font files to latest version, 2.09. See #24595.

comment:12 @lancewillett2 years ago

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

In 24516:

Twenty Thirteen: enqueue Genericons CSS from its own file, to allow for easier updates and maintenance, and to allow for better plugin overrides. Props obenland, closes #24595.

@DrewAPicture2 years ago

Load genericons as an editor stylesheet

comment:13 @DrewAPicture2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Looks like when we moved Genericons to its own file in [24516], they stopped working in the Visual editor:

http://f.cl.ly/items/32171W151S2B1I3t290d/Screen%20Shot%202013-07-02%20at%2011.25.02%20AM.png

24595.2.patch adds genericons.css as an editor style:

http://f.cl.ly/items/2Z3i1X1U262b441Y1I2u/Screen%20Shot%202013-07-02%20at%2011.25.51%20AM.png

comment:14 follow-up: @nacin2 years ago

Sidenote, after seeing that screenshot: I'm all for editor styles but I'm not sure if the background color being used for reading is always going to be as conducive for writing?

comment:15 @DrewAPicture2 years ago

Also in retrospect, moving genericons into its own stylesheet (from style.css) shouldn't have had any effect on whether genericons worked in the editor. I can't figure out how the editor styles were previously inheriting the Genericon font family.

Related-ish: #24308

comment:16 in reply to: ↑ 14 @DrewAPicture2 years ago

Replying to nacin:

Sidenote, after seeing that screenshot: I'm all for editor styles but I'm not sure if the background color being used for reading is always going to be as conducive for writing?

From an accessibility standpoint, as long as the contrast ratio is in compliance with the WCAG2 guidelines for reading on the front-end (see #23513) I don't see a problem with matching it for writing that has to be read on the back-end. I see your point though.

Semantics aside, I think it's another nice touch that sets this theme apart.

@DrewAPicture2 years ago

comment:17 @DrewAPicture2 years ago

24595.3.patch passes the editor styles as an array.

comment:18 @lancewillett2 years ago

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

In 24541:

Twenty Thirteen: enqueue Genericons CSS file for use in visual editor styles. Props DrewAPicture, closes #24595.

Note: See TracTickets for help on using tickets.