Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#21694 closed defect (bug) (fixed)

Twenty Twelve - Customize: Enable Open Sans Typeface

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

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

Download all attachments as: .zip

Change History (24)

  • Milestone changed from Awaiting Review to 3.5

I agree.

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

Last edited 9 months ago by nacin (previous) (diff)

comment:2 follow-ups: ↓ 3 ↓ 4   nacin9 months ago

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

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

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

Replying to nacin:

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

I'm OK with that.

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: ↓ 6   matveb9 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   leogermani9 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. ;)

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.

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.

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.

Enable body_class with wp_script_is check

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

Sorry, wp_style_is().

Enable body_class for custom font with wp_style_is check

comment:12 follow-up: ↓ 13   obenland9 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: ↓ 14   nacin9 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   lancewillett9 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.

Adds oldskool example for dequeueing the fonts style

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."

:)

Adds second param for wp_style_is() back in for clarity

  • 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.

  • 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?

  • 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.

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

Note: See TracTickets for help on using tickets.