WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#21694 closed defect (bug) (fixed)

Twenty Twelve - Customize: Enable Open Sans Typeface

Reported by: viniciusmassuchetto Owned by: lancewillett
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4.1
Component: Bundled Theme Keywords:
Focuses: Cc:

Description

On theme customize for Twenty Twelve, the option "Enable the Open Sans typeface" will only actually change the site font if the user doesn't have the Open Sans font on their system. So for people that have Open Sans, the option won't do anything.

If the desired behavior for this option is to switch between Helvetica/Open Sans, it should always change the body font stack.

Attachments (5)

opensans.diff (1.4 KB) - added by viniciusmassuchetto 20 months ago.
Adds an 'enable-font' class to the body when using Open Sans font option.
21694.diff (1.5 KB) - added by lancewillett 20 months ago.
Enable body_class with wp_script_is check
21694.2.diff (1.6 KB) - added by lancewillett 20 months ago.
Enable body_class for custom font with wp_style_is check
21694.3.diff (1.6 KB) - added by obenland 20 months ago.
Adds oldskool example for dequeueing the fonts style
21694.4.diff (1.7 KB) - added by lancewillett 20 months ago.
Adds second param for wp_style_is() back in for clarity

Download all attachments as: .zip

Change History (24)

comment:1 nacin20 months ago

  • Milestone changed from Awaiting Review to 3.5

I agree.

I'm close to proposing we rip it out and just always go with Open Sans. That then saves us from a lot of extra code in 2012.

Version 0, edited 20 months ago by nacin (next)

comment:2 follow-ups: nacin20 months ago

I recognize that my comment conflicts my original thoughts in #19978.

viniciusmassuchetto20 months ago

Adds an 'enable-font' class to the body when using Open Sans font option.

comment:3 in reply to: ↑ 2 lancewillett20 months ago

Replying to nacin:

I recognize that my comment conflicts my original thoughts in #19978.

I'm OK with that.

comment:4 in reply to: ↑ 2 viniciusmassuchetto20 months ago

Replying to nacin:

I recognize that my comment conflicts my original thoughts in #19978.

I like the idea of letting people choose which font they want and keep Helvetica as a default one. I'm not completely into the previous discussions about it, apart from the ones in #19978.

It can be quite a specific opinion, but one thing I don't like is to rely on Google for fonts -- even recognising it is averagely faster. I would like to be able to disable this option in my site.

comment:5 follow-up: matveb20 months ago

Setting aside the discussion about the option's existence, I think a body class to narrow the font stack is an elegant approach—keeps it all clean on style.css.

comment:6 in reply to: ↑ 5 leogermani20 months ago

Replying to matveb:

Setting aside the discussion about the option's existence, I think a body class to narrow the font stack is an elegant approach—keeps it all clean on style.css.

me too. ;)

comment:7 lancewillett20 months ago

After discussion in IRC with Nacin today, we're going to:

  1. Remove all theme options code and support
  2. Make Open Sans the default font, first in the stack and always loaded
  3. Move customizer pieces from Theme Options out of class structure and into functions.php

I don't think make the font load based on body class is the ideal. To disable it users can use a few lines of code in their child theme, and later we can consider adding a filter so child themes can prevent the Google CSS font from loading without unhooking the entire twentytwelve_scripts_styles action.

comment:8 lancewillett20 months ago

In [21639]:

Twenty Twelve: simplify custom font loading behavior, see #21694.

  • Remove theme options code and support from theme.
  • Keep Open Sans font first in the stack and make it always loaded.
  • Move Customizer pieces from Theme Options out of class structure and into functions.php.

comment:9 lancewillett20 months ago

A more elegant solution, incorporating the default font with a body_class value, from Nacin tonight in IRC.

add a body class like body.open-sans in twentytwelve_body_class() only if wp_script_is( 'enqueue' ) for twentytwelve-fonts.
then just dequeueing the font makes the whole thing vanish.

https://gist.github.com/3494376

To dequeue in a child theme:

add_action( 'wp_enqueue_scripts', function() { wp_dequeue_style( 'twentytwelve-fonts' ); }, 11 );

See full IRC chat log.

lancewillett20 months ago

Enable body_class with wp_script_is check

comment:10 lancewillett20 months ago

21694.diff implements the body_class idea -- except in testing it doesn't work. wp_script_is appears to only verify scripts, not styles.

comment:11 nacin20 months ago

Sorry, wp_style_is().

lancewillett20 months ago

Enable body_class for custom font with wp_style_is check

comment:12 follow-up: obenland20 months ago

I really like this approach. Also showcases the wp_script_is() function, very few will know about.

In the dequeue example I'd prefer a pre-PHP 5.3 syntax for new developers, though. What do you think?

comment:13 in reply to: ↑ 12 ; follow-up: nacin20 months ago

Replying to obenland:

I really like this approach. Also showcases the wp_script_is() function, very few will know about.

In the dequeue example I'd prefer a pre-PHP 5.3 syntax for new developers, though. What do you think?

The one-liner was just me in IRC. It should definitely be changed.

I would like to specify a second argument to wp_style_is(), 'enqueued'. By default it is 'queue' (same) but it reads much better as a proper sentence. I am going to update the function to take a better argument in another ticket.

comment:14 in reply to: ↑ 13 lancewillett20 months ago

Replying to nacin:

In the dequeue example I'd prefer a pre-PHP 5.3 syntax for new developers, though. What do you think?

The one-liner was just me in IRC. It should definitely be changed.

Suggestions welcome.

I would like to specify a second argument to wp_style_is(), 'enqueued'. By default it is 'queue' (same) but it reads much better as a proper sentence.

OK, I removed the second argument since it's the default argument. I like "enqueued" and reading as a real phrase, nice.

obenland20 months ago

Adds oldskool example for dequeueing the fonts style

comment:15 lancewillett20 months ago

Noting so we don't forget later, we'd like to use wp_is_style() but patch core to change the argument value to 'enqueue' instead of 'queue' and use the default value in Twenty Twelve so it reads like a sentence:

— "WP, is style enqueued?"
— "Why yes, Konstantin — glad you asked."

:)

lancewillett20 months ago

Adds second param for wp_style_is() back in for clarity

comment:16 lancewillett20 months ago

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

In [21668]:

Twenty Twelve: move Open Sans custom font loading to a body class to allow easier overriding, fixes #21694.

  • Use wp_style_is() to check for the enqueued CSS file before adding the body class value.
  • Add a sample dequeue method in the comments as documentation for child themers and their ilk.

Props viniciusmassuchetto, nacin, and obenland.

comment:17 pavelevap20 months ago

  • Cc pavelevap@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Open Sans probably does not have support for Czech characters, so after activating Twenty Eleven (latest trunk), all characters like ž, š, č, etc. are not visible on website. There should be any settings for translators to disable this font by default?

comment:18 nacin20 months ago

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

pavelevap: can you open a new ticket for that? We'll figure out a good solution.

comment:19 pavelevap20 months ago

Sure, I hesitate to reopen this or create a new one :-)

Note: See TracTickets for help on using tickets.