#20448 closed task (blessed) (fixed)
Update Twenty Ten and Twenty Eleven to use 3.4 features
Reported by: | nacin | Owned by: | koopersmith |
---|---|---|---|
Milestone: | 3.4 | Priority: | high |
Severity: | major | Version: | 3.4 |
Component: | Bundled Theme | Keywords: | has-patch |
Focuses: | Cc: |
Description (last modified by )
Twenty Eleven can be given three distinct updates for 3.4:
- Flexible heights for headers (when a post thumbnail header is not in use)
- Adding Twenty Eleven's theme options to customize
- Providing Text Domain and Domain Path headers
Additionally, a theme just doesn't need a $locale.php file — indeed, core almost never needs one either anymore. I suggest we remove the reference, something I had already done for Twenty Twelve when it was in trunk.
Attachments (24)
Change History (97)
#3
@
13 years ago
- Keywords has-patch added
- Owner set to koopersmith
- Status changed from new to reviewing
#7
follow-up:
↓ 8
@
13 years ago
- Component changed from Themes to Bundled Theme
- Summary changed from Update Twenty Eleven to use 3.4 features to Update Twenty (Ten|Eleven) to use 3.4 features
#10
@
13 years ago
- Priority changed from normal to high
- Severity changed from normal to major
- Summary changed from Update Twenty (Ten|Eleven) to use 3.4 features to Update Twenty Ten and Twenty Eleven to use 3.4 features
twentyeleven-customize.diff:ticket:19910 from Otto42 adds postMessage support for Twenty Eleven.
@
13 years ago
Patch for twentyeleven to support postMessage updating of title, description, and header text-color
#13
follow-up:
↓ 14
@
13 years ago
New patch adds color scheme and link color choices to the customizer.
The link color stuff is partially broken because twentyeleven stores the link color in the database with the # in front of it, while the customizer's color control expects it to not have that. I wrote a function to add the # to make the previewer work, but the control appears wonky when you first load the page.
Uploading the patch anyway in case somebody can see a better solution than myself. I think we'll probably need to make the color control capable of handling the data value with or without the # on the color to get the widest use out of it.
#14
in reply to:
↑ 13
@
13 years ago
Replying to Otto42:
New patch adds color scheme and link color choices to the customizer.
I take it you might have missed 20448.diff :-) Looks good. Obviously far more comprehensive than my initial attempt. I ran into the same issue with #, and I think koopersmith said he was going to work on that.
#15
follow-up:
↓ 23
@
13 years ago
Yeah, I did miss that. :)
Patch 3 adds layouts using much the same method nacin's patch used, but with the addition of calling twentyeleven_layouts() to get the list of layouts instead of hardcoding them.
#16
@
13 years ago
Just a note that header_textcolor
is also the checkbox that should show/hide the header text. Hidden when "blank" or empty string. Also see #20600
#19
follow-up:
↓ 25
@
13 years ago
I've hit a bug where the default "full page refresh" preview of an option doesn't work unless I already have values saved in the DB for the theme.
So, to replicate:
- Delete the
twentyeleven_theme_options
value from your database. - Open the customizer for TwentyEleven and toggle the dark/light color scheme, and it won't update.
- Click "Save" on the customizer pane to save the current options.
- Try toggling the color scheme again, and it will work.
It looks like the customizer is expecting a value to be in the DB? This doesn't have to do with saving defaults in the database, does it?!?
#21
@
13 years ago
andyadams: Patch that should correct this problem is now in #19910. However, I'm not sure that it's the final version of the patch that will be used. But I can confirm that the problem exists.
#23
in reply to:
↑ 15
@
13 years ago
Replying to Otto42:
Patch 3 adds layouts using much the same method nacin's patch used, but with the addition of calling twentyeleven_layouts() to get the list of layouts instead of hardcoding them.
Would it make sense to move the JavaScript into its own file, and enqueue it via the 'wp_enqueue_scripts'
hook? Works well for me.
#24
follow-up:
↓ 5
@
13 years ago
- Description modified (diff)
#15858 — we also should add Text Domain and Domain Path headers until a subsequent phase of i18n ideally renders those unnecessary.
#25
in reply to:
↑ 19
@
13 years ago
Replying to andyadams:
It looks like the customizer is expecting a value to be in the DB? This doesn't have to do with saving defaults in the database, does it?!?
It seems that Customize is using a pre_option_*
filter to short-circuit string options, but the option_*
filter when it comes to arrays. Since the array is never stored in the database, the option_*
filters are never reached because get_option
will return the default without passing it through the filter. By simply replacing return $default
with $value = $default
in get_option
I'm able to get the override of non-existing options to work, but I have a feeling this can break something else.
#27
in reply to:
↑ 26
@
13 years ago
Otto42: Aye, that's brilliant! Sorry I missed that before writing my comment above. Thanks!
#29
@
12 years ago
Per bug scrub, let's add a default option filter to all of our option functions and add unit tests.
#32
@
12 years ago
Note on [20783]: The placement of the filter means that the _preview_filter function will run twice when the option exists, once on the default_option value, and once on the real option value. This may lead to unintended consequences in the future, although I don't think it breaks anything right away.
#34
@
12 years ago
20448.twentyeleven.4.diff, for Twenty Eleven:
- Moves Customizer integration code to theme options PHP file, and minor WP code standards cleanup (spaces, alignment)
- Adds comment blocks to new functions
- Moves JS to external file
Note from testing: the header_textcolor
postMessage callback to change the text color via .css()
doesn't work after a custom color is set.
It only works the first time you override the default value. I think it's something with the !important
set in functions.php for the header color (see http://core.trac.wordpress.org/browser/trunk/wp-content/themes/twentyeleven/functions.php#L221).
#37
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Already pinged koop, but re-opened so it doesn't get lost.
Issue with the link color: http://cl.ly/Gtg6 (notice the two #)
#5
in reply to:
↑ 24
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
#6
@
12 years ago
- Cc pavelevap@… added
Also theme tags strings should be removed from .pot files (and GlotPress), for example: http://translate.wordpress.org/projects/wp/dev/twentyeleven/cs/default?filters[status]=either&filters[original_id]=19426&filters[translation_id]=1115234
Or should I create another ticket?
#6
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Twenty Eleven and Twenty Ten should additionally specify a default-color argument when calling add_custom_background().
Since the behavior of default colors and images for backgrounds are not well supported (UI-wise) prior to 3.3, we should not bother to maintain compatibility (see #20768) by defining the associated constants.
@
12 years ago
Specify default-color argument when calling add_custom_background() in Twenty Ten and Twenty Eleven. :)
#7
follow-up:
↓ 8
@
12 years ago
20448.5.diff handles the default colors for both Twenty Ten and Twenty Eleven, including both of Twenty Eleven's color schemes.
It also changes the default custom background callback to only operate on saved values, rather than default values. This prevents an unsaved default value from overriding a manually modified style.css file. body.custom-background should only be reflected when there's actually a custom background in play.
With regards to 20448.4.diff, #fff is the CSS reset background color, not what we specify later in each stylesheet.
#8
in reply to:
↑ 7
@
12 years ago
Replying to nacin:
With regards to 20448.4.diff, #fff is the CSS reset background color, not what we specify later in each stylesheet.
Right, I thought it was more about having the argument visible in functions.php
, which is why I tried to use what I considered the least obtrusive color. Didn't even think of the dark color scheme :(
#10
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
koopersmith and I agreed in IRC to avoid the conditional from beyond. A patch just never made it here.
It should be a simple if ( get_theme_mod( 'background_image' ) || get_theme_mod( 'background_color' ) )
But we should double-check the IRC logs to see if we missed something in our thought process.
#11
@
12 years ago
Currently themes that don't hard code the default image in css don't get the default background image applied like they did prior to 20973. If they switch to hard coding, they lose the ability to "Remove Background Image".
#12
@
12 years ago
20448-remove-bg-image.2.diff will allow removing the background in the current version of Adventure Journal, which has switched to hard coding the default background image in the CSS. It will not fix displaying the default image for the 3.3 era AJ, which did not hard code.
#13
follow-up:
↓ 14
@
12 years ago
Combining the patches gets us closer. It works for default and no bg image whether the bg image is in css or not.
#14
@
12 years ago
If a theme does this:
background: #693808 url( "images/mp-background-tile.jpg" );
Then if the user removes both the background and the color via the custom header admin screen, the default bg image will still show.
#15
follow-up:
↓ 23
@
12 years ago
If the theme registers a default background color, a "Default" link shows next to the color picker. If the color has been emptied, this link no longer shows. The show_clear logic needs to be cleaned up for when a default color is registered but the current color is empty.
#19
follow-up:
↓ 25
@
12 years ago
Okay, let's bury the idea that, if un-saved defaults are in place, there should be no <style> block printed. This hurts themes that used BACKGROUND_IMAGE without specifying a fallback in the stylesheet, and they did that because otherwise removal wouldn't work.
20448.8.diff reverts wp-includes for [20973] and then makes some adjustments:
- <style> will appear if there is a default image registered. This is the same as 3.3.
- If only a default color is registered, it still assumes it is in the stylesheet, and no <style> will appear. This is a change from 3.3.
- <style> will continue to appear as before if there is a custom background color or image. This is the same as 3.3.
- This then allows for a default background image with background-image: none, overriding style.css. This is new.
The goals of [20973] were to make default-image and default-color not suck, thus encouraging their use, since they're basically new in 3.4.* But at the same time we don't want to encourage people to stop putting these things in CSS as well. We simply want them to register it with WordPress so we know.
(*) I consider them new because:
- They are now first-class arguments, rather than second-class constants. People will notice them.
- The constants did not work well, as they could not be removed, were not part of the preview, and were not represented in the UI.
- The default themes are now leveraging them.
This does not fix comment 15.
#21
@
12 years ago
20448.10.diff addresses this:
If the theme registers a default background color, a "Default" link shows next to the color picker. If the color has been emptied, this link no longer shows. The show_clear logic needs to be cleaned up for when a default color is registered but the current color is empty.
Purely JS, as the field cannot be blank in PHP (the default would get filled in).
#24
follow-up:
↓ 5
@
12 years ago
- Resolution set to fixed
- Status changed from reopened to closed
In [21004]:
#25
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
We missed the customizer in this. Patch attached. Using a setting may not have been the most appropriate way to pass a value to the JS, but it does the job.
#27
@
12 years ago
20448.12.diff works. Good for commit.
#29
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
There are still some issues with body.custom-background since get_background_color() will always return true for themes with a registered default. So, the check should be:
if ( get_theme_mod( 'background_color' ) || get_theme_mod( 'background_image' ) || get_theme_support( 'custom-background', 'default-image' ) ) ...
Problem reported by Frumph: http://lists.automattic.com/pipermail/wp-testers/2012-June/014487.html
There also seem to be some issues when looking at _custom_background_cb():
- $background includes the default-image, so the default-image check on line 1112 is redundant
- $background includes the default-image, so the "there is not a $background, but there is a default" branch will never be reached
It seems like switching $background back to just the saved image would fix this:
$background = get_theme_mod( 'background_image' );
#30
@
12 years ago
The logic for custom backgrounds is correct.
- If a custom background image is set, get_background_image() will return the theme mod.
- If a custom background image is not set and there is no default value, get_background_image() will evaluate to false.
- If a custom background image is set but empty, this means the image has been "removed", and the empty value would not be overridden by a default. This means get_background_image() (and $background) would evaluate to false.
- In the case of a set-but-empty image ("removed"), where there is a default image, we print out background-image: none.
- Thus, the custom-background body class shows up whenever there is either a background image, or a default image.
- The report from Frumph suggests that all customizations were removed, but to remove all customizations, you need to "Reset" rather than "Remove."
Custom colors do have one bug, and I think it was just a transposition issue:
- If there is no custom color saved, then get_background_color() will return the default. However, this situation should not output
background-color: (default)
in the CSS, as this can override a modified stylesheet. (So can images, but that is less of a concern.) - It does not output background-color, but it *does* output the custom-background body class. This is incorrect.
- Wrong:
get_background_color() || get_theme_mod( 'background_image' )
. Correct:get_theme_mod( 'background_color' ) || get_background_image()
. - Since we subsequently also check for
get_theme_support( 'custom-background', 'default-image' )
, there is no functional difference betweenget_theme_mod('background_image')
orget_background_image()
here. But you can now see why it was a transposition issue — the wrong one was moved to be a theme_mod check.
#31
@
12 years ago
20448.13.diff corrects the logic for adding the custom-background body class.
#32
@
12 years ago
Looks good. Tested with Twenty Eleven, Twenty Ten, and old and new Adventure Journal.
#33
@
12 years ago
See also 20132.diff:ticket:20132, which includes 20448.13.diff.
#35
@
12 years ago
With the changes above, the default background-color does not get applied after initial activation of a Theme. The user has to navigate to the custom-backround admin screen, to explicitly save the (proposed) color, so it gets picked up by get_theme_mod( 'background_color' )
.
Are Themes supposed to set a background color in style.css
on top of specifying 'default-color'
when adding theme support?
Thanks,
Konstantin
In [20470]: