#18887 closed enhancement (fixed)
Appearance -> Header UI: "Display Text" Needs Distinct theme_mod
Reported by: | chipbennett | Owned by: | |
---|---|---|---|
Milestone: | 3.4 | Priority: | normal |
Severity: | minor | Version: | 3.2 |
Component: | UI | Keywords: | has-patch close |
Focuses: | Cc: |
Description
The current UI on the Appearance -> Header admin screen is misleading with respect to the "Display Text" form field. This field does not have a distinct theme_mod, but rather correlates to HEADER_TEXTCOLOR
. From `wp-admin\custom-header.php`:
<td> <p> <?php $hidetext = get_theme_mod( 'header_textcolor', HEADER_TEXTCOLOR ); ?> <label><input type="radio" value="1" name="hidetext" id="hidetext"<?php checked( ( 'blank' == $hidetext || empty( $hidetext ) ) ? true : false ); ?> /> <?php _e( 'No' ); ?></label> <label><input type="radio" value="0" name="hidetext" id="showtext"<?php checked( ( 'blank' == $hidetext || empty( $hidetext ) ) ? false : true ); ?> /> <?php _e( 'Yes' ); ?></label> </p> </td>
The issue is with this line:
<?php $hidetext = get_theme_mod( 'header_textcolor', HEADER_TEXTCOLOR ); ?>
Conditionally displaying text based on the theme_mod for header_textcolor
being defined or not is unintuitive. There is no practical reason that the theme_mod to display/not display header text should be tied to the theme_mod specifying the header text color. Yet, currently, in order to support the "Display Text" field, header text output must be wrapped in a if ( 'blank' != get_header_textcolor() )
conditional.
I think the simplest solution would be to create a distinct theme_mod
for display_text
, with a corresponding display_header_text()
conditional.
(See also ticket #17605)
Attachments (3)
Change History (36)
#2
@
13 years ago
I agree that using a 'blank' color to disable the header text is hackish.
But maybe we can just create the display_header_text() conditional that hides the hack.
#3
@
13 years ago
That would at least be a good first step, if nothing else!
Should I just create the conditional, so that it available to Themes, or should it be propagated throughout the Admin where applicable?
#6
@
13 years ago
Okay, I'll generate a patch for the core files, then one for Twenty Eleven. Is it okay if I do iterative patches, and get feedback for each step? This will be the most I've modified any core files, so I want to be sure I do it correctly.
#9
follow-up:
↓ 10
@
13 years ago
The function description is too verbose. I would replace it with just "Conditional check to determine if the user has enabled the Display Text option..
The function implementation is also too verbose. It should be a single line:
return 'blank' != get_header_textcolor();
#10
in reply to:
↑ 9
@
13 years ago
Replying to scribu:
The function description is too verbose. I would replace it with just
Conditional check to determine if the user has enabled the Display Text option.The function implementation is also too verbose. It should be a single line:
return 'blank' != get_header_textcolor();
Made less-verbose, as per your suggestion.
#11
@
13 years ago
- Summary changed from Appearnce -> Header UI: "Display Text" Needs Distinct theme_mod to Appearance -> Header UI: "Display Text" Needs Distinct theme_mod
#12
@
13 years ago
And here's the patch for \wp-admin\custom-header.php
, using the new display_header_text()
conditional function.
#13
@
13 years ago
Just a note that, while doing some testing of the new code, I'm still noticing an issue that results from the conflation of header_textcolor
and display_header_text
. If "Display Text" is set to false
, and then changed to true
, the header_textcolor
displays as #blank
, which not only is semantically incorrect (the value is expected to be a valid, six-digit HEX color value), but also poor UX (the previous header_textcolor
apparently isn't retained when "Display Text" is set to false
).
So, even after putting an intuitive display_header_text()
around the header_textcolor
hack, we've still not solved all of the non-intuitive UX issues.
#15
in reply to:
↑ 14
@
13 years ago
Replying to scribu:
I can't reproduce that on current trunk.
While trying to reproduce the observation, I discovered something else: If "display header text" is set to false
, and then the "Restore Original Header Text" button is clicked, the original header text settings are restored, but the UI does not change (javascript issue?). That is: the Text Color text field remains hidden, until the page is refershed.
Steps to reproduce:
1) Ensure that "display header text" is set to false
2) Select "restore original header text"
Is this related enough to keep in this ticket, or should I submit a different ticket?
#16
@
13 years ago
When I click the "Restore Original header text" button, a page refresh ensues and everything works as expected.
There doesn't seem to be any JS involved. Tested on Twentyeleven.
#17
@
13 years ago
Interesting.
In the most recent nightly, it appears that the "Display Header Text" radio buttons have been removed entirely. Now, the "Text Color" text field label reads, If you want to hide header text, add #blank as text color.
So, I guess that works, too. (Though, I don't think the instruction to use #blank
either semantic or intuitive.)
#18
@
13 years ago
Oops; disregard previous comment. Newly mapped domain name, and forgot to enable javascript for the domain in Chrome. :/
#19
@
13 years ago
@scribu
Okay, after a bit of testing with Twenty Eleven, I think I understand why you can't recreate the issues I'm describing: Twenty Eleven defines NO_HEADER_TEXT
as true
`'.
The wonky issues thus only appear for Themes that define NO_HEADER_TEXT
as false
- that is, that display the Header Text color field by default. Since Twenty Eleven doesn't display this field by default, the issue doesn't appear. But for Themes that do display this field, it appears not to be displayed when clicking "Restore Original Header Text".
#20
@
13 years ago
Could you give an example of such a theme that I can install from the comfort of my wp-admin? (I'm lazy :P)
#21
@
13 years ago
I would say to install Oenology, but all of my header-text fixes are in the soon-to-be-released Version 2.5; so it's not quite yet available in the repo. Maybe I can finish it up this weekend and get it submitted, so you can one-click install it. :)
Otherwise, you can get the development version from GitHub.
#24
@
13 years ago
- Severity changed from normal to minor
- Type changed from defect (bug) to enhancement
You should make a single patch that combines themes.php.display_header_text.rev2.diff
and custom-header.php.display_header_text.diff
Also, {1-2 words}.{ticket number}.diff
is a suficiently descriptive patch name.
#28
@
13 years ago
- Keywords close added; needs-testing removed
- Milestone changed from Awaiting Review to 3.4
get_header_textcolor() is always going to need to return 'blank' for compatibility reasons. I tweaked it so there is now a new function, display_header_text(), and I leveraged that function in core.
However, the 'blank' checks are still in Twenty Eleven. I don't see a reason to go from:
if ( 'blank' == $text_color ) :
To something like:
if ( ( function_exists( 'display_header_text' ) && display_header_text() ) || 'blank' == $text_color ) :
I'll be happy to try to write a patch for this, but it has some deep fingers in
wp-admin\custom-header.php
. If the idea has some consensus, I'll try to tackle it.