WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 6 months ago

#25220 closed feature request (fixed)

Twenty Fourteen: Let Users Customize the Green "Accent" Color

Reported by: celloexpressions Owned by:
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.8
Component: Bundled Theme Keywords:
Focuses: Cc:

Description

Continued from #25094.

Attachments (8)

25220.4.diff (6.1 KB) - added by celloexpressions 8 months ago.
Refreshed for social links removal - [25214]
25220.5.diff (7.1 KB) - added by celloexpressions 7 months ago.
Formatting fixes
25220.6.diff (7.1 KB) - added by lancewillett 7 months ago.
25220.mediaelements-fix.diff (516 bytes) - added by celloexpressions 7 months ago.
Apply the primary accent color to the current-time indicator for the mediaelements players (default accent color added there while initial patch was in progress).
25220.hover-state-fixes.diff (898 bytes) - added by celloexpressions 7 months ago.
Fix several instances of the custom accent color CSS overriding white or black link colors that should have no text-color change on hover by adding link color declarations to additional and more specific selectors.
25220.hader-nav-hover-pattern-change.diff (1.0 KB) - added by celloexpressions 7 months ago.
Remove the text colors from the header nav hover states for sub-menus, and prevent custom accent colors from adding it to top-level items, per @iamtakashi's suggestions; change the background color more on hover for submenus.
25220.header-submenu-hover.2.diff (435 bytes) - added by celloexpressions 7 months ago.
Try a darker text color for header nave submenu items on hover (matching site description color).
25220.hover-state-fixes.2.diff (429 bytes) - added by celloexpressions 7 months ago.
Fixes another bad hover color override.

Download all attachments as: .zip

Change History (32)

celloexpressions8 months ago

Refreshed for social links removal - [25214]

comment:1 lancewillett8 months ago

  • Milestone changed from Awaiting Review to 3.8

comment:2 lancewillett8 months ago

As discussed in today's office hours (log) we're going to remove the ability to change the header text color in favor of this more general accent color.

Last edited 8 months ago by lancewillett (previous) (diff)

comment:3 obenland8 months ago

Patch looks good, only some minor formatting issues:

  • Use spaces to align things in a block of code.
  • Comments should always have the first letter capitalized and end with a period.
  • Functions should have (at least) arguments and return value documents with PHPDoc (no trailing empty line necessary)

As mentioned during chats, we should remove the priority argument and find better names for accent_1 and accent_2.

celloexpressions7 months ago

Formatting fixes

comment:4 celloexpressions7 months ago

25220.5.diff addresses the formatting fixes. We still need to discuss the naming conventions, though; I think we can do better than accent_lighter and accent_much_lighter.

comment:5 follow-up: obenland7 months ago

Patch .5 looks really good, but I agree, accent_* names should be further iterated on.

wp_add_inline_style expects the CSS string not to include any HTML, IIRC.

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

Replying to obenland:

wp_add_inline_style expects the CSS string not to include any HTML, IIRC.

That was only the case with SCRIPT_DEBUG enabled (until [25202]), see #24813.

lancewillett7 months ago

comment:7 lancewillett7 months ago

Tiny formatting things in .6 (just for reference on what I changed):

  • Add spaces after if statement word before braces
  • Yoda-style comparison for if ( '#24890d' === $accent_color ) and changed to === for strict comparison
  • Cleaned up trailing spaces

comment:8 lancewillett7 months ago

In 25418:

Twenty Fourteen: let authors customize the green "accent" color by changing to a different hex value in the Customizer. Props celloexpressions.

Also add an ID attribute to all style elements.

See #25220.

comment:9 celloexpressions7 months ago

Next steps:

  • Look into applying the primary accent color to links and highlight color in TinyMCE.
  • Add selectors to adjust the colors on the mediaelement players and anywhere else they've been added since this initial patch was started.
  • Make sure all Twenty Fourteen patches touching non-grayscale colors and/or their selectors also make the necessary edits to the <style> block in customizer.php.
  • Fix discrepancies between color patterns with and without a custom accent color; for example, the hover color of top-level header nav items (note that this one also seems to have been overridden with child themes of Further, for example, on http://themeshaper.com). I think paging navigation also needed some attention.
  • Consider adjusting a couple of color patterns; for example, the header nav sub-menu item hover color is the darkest (original) accent color, displayed against a black background, we should consider changing this to the lightest generated color like the other link hover on black background items. This is the only place where that color is used on a black background, which limits the ability to use darker accent colors. Might be worth having @iamtakashi take a look at it with a few alternate accent colors and provide direction on anywhere we should adjust the use of the different generated colors (or adjust the generation process).
  • Remove the ability to change the "header text color" -- which is really only the site title text color. If we want to keep the ability to hide that text, we'll need to add the ability to remove the color control and keep the other header controls to core (and it should definitely be there), which I can look into.
  • Come up with better names for accent_lighter and accent_much_lighter. The variables don't matter much, but those labels are also used for the theme_mods.

comment:10 iamtakashi7 months ago

  • Cc takashi@… added

celloexpressions7 months ago

Apply the primary accent color to the current-time indicator for the mediaelements players (default accent color added there while initial patch was in progress).

celloexpressions7 months ago

Fix several instances of the custom accent color CSS overriding white or black link colors that should have no text-color change on hover by adding link color declarations to additional and more specific selectors.

celloexpressions7 months ago

Remove the text colors from the header nav hover states for sub-menus, and prevent custom accent colors from adding it to top-level items, per @iamtakashi's suggestions; change the background color more on hover for submenus.

comment:11 celloexpressions7 months ago

Just uploaded several patches that address points from the to-do list. The only one that really needs discussion is the one that changes the hover-state colors in the header nav menu. Particularly, for the sub-menus we decided to try retaining the white text color and adjusting the background color to give it more contrast from the top-level items. Since we couldn't go any darker for that hover color (background was #000), I tried going lighter, slightly lighter than the top-level items go.

I think the best way to make sure we don't miss any colors in customizer.php is to strongly encourage all theme testers/contributors to pick a custom color and leave it enabled. That way, people will notice bugs with that component specifically (like the paging ones patched above) as well as remembering to address it when modifying other things. The theme should be polished regardless of whether this customization is in use, so we might as well test it with custom colors.

comment:12 lancewillett7 months ago

In 25612:

Twenty Fourteen: fixes for accent color functionality, props celloexpressions. See #25220.

  • Apply the primary accent color to the current-time indicator for the mediaelements players.
  • Fix instances of the custom accent color CSS overriding white or black link colors that should have no text-color change on hover.
  • Remove text color from main navigation hover states for submenus and from main navigation top-level items.
  • Change the background color a bit more on hover for main navigation submenus.

comment:13 follow-ups: celloexpressions7 months ago

The commit message doesn't match the commit :)

You did the first two, but not the header nav stuff, and that needs discussion (or approval from iamtakashi) first anyway.

comment:14 in reply to: ↑ 13 ; follow-up: iamtakashi7 months ago

Replying to celloexpressions:

... the header nav stuff, and that needs discussion (or approval from iamtakashi) first anyway.

We could match the background color of hover state for submenus the same as the top level: #2b2b2b rather than introducing another shade of gray. Then we can also remove line 713 .primary-navigation li li:hover > a.

comment:15 in reply to: ↑ 13 lancewillett7 months ago

Replying to celloexpressions:

The commit message doesn't match the commit :)

You did the first two, but not the header nav stuff, and that needs discussion (or approval from iamtakashi) first anyway.

You're right, I meant to apply it and let you both iterate in subsequent patches, but didn't end up applying it after I'd written the commit message.

comment:16 lancewillett7 months ago

In 25623:

Twenty Fourteen: change the background color a bit more on hover for main navigation submenus. See #25220.

comment:17 in reply to: ↑ 14 ; follow-up: celloexpressions7 months ago

Replying to iamtakashi:

Replying to celloexpressions:

... the header nav stuff, and that needs discussion (or approval from iamtakashi) first anyway.

We could match the background color of hover state for submenus the same as the top level: #2b2b2b rather than introducing another shade of gray. Then we can also remove line 713 .primary-navigation li li:hover > a.

I think having something different for the submenus helps guide the visual hierarchy, but maybe we could try accomplishing that by using a darker text color. I'll add another patch which does that, matching the text color of the site description (which should probably be converted from rgba to hex, by the way. It's #8c8c8c I think).

celloexpressions7 months ago

Try a darker text color for header nave submenu items on hover (matching site description color).

celloexpressions7 months ago

Fixes another bad hover color override.

comment:18 in reply to: ↑ 17 ; follow-up: iamtakashi6 months ago

Replying to celloexpressions:

I think having something different for the submenus helps guide the visual hierarchy, but maybe we could try accomplishing that by using a darker text color. I'll add another patch which does that, matching the text color of the site description (which should probably be converted from rgba to hex, by the way. It's #8c8c8c I think).

I've tested 25220.header-submenu-hover.2.diff​ but I don't like the low contrast text color for the hover state with the grey background.

Another idea is that we can match the style with the secondary navigation on the left so that both navigations have the same (or very close) patterns.

comment:19 in reply to: ↑ 18 celloexpressions6 months ago

Replying to iamtakashi:

Another idea is that we can match the style with the secondary navigation on the left so that both navigations have the same (or very close) patterns.

The only issue with that is that it brings us back to the problem of lessening the distinction of the current page. I agree on the low contrast though.

I think our best option is probably to restore the original behavior but switch to the lightest accent color on hover (matching the sidebar, and reverting to the black background color), or possibly trying the gray text color against the black background (which I believe is the last option we haven't tried yet).

Sidenote: 25220.hover-state-fixes.2.diff is ready to go in, and grabs another bad hover state I missed earlier.

comment:20 lancewillett6 months ago

In 25730:

Twenty Fourteen: accent color fixes, props celloexpressions. See #25220.

  • Try a darker text color for header nave submenu items on hover (matching site description color).
  • Fix a bad hover color override.

comment:21 lancewillett6 months ago

  • Keywords close added; has-patch removed

@celloexpressions Can we close this ticket and open new ones as needed?

comment:22 follow-up: celloexpressions6 months ago

Remaining items left:

  • Come up with better names for accent_lighter and accent_much_lighter. The variables don't matter much, but those labels are also used for the theme_mods.
  • Look into applying the primary accent color to links and highlight color in TinyMCE.
  • Remove the ability to change the "header text color" -- which is really only the site title text color. See #25540

First item should definitely happen here, if we're going to adjust it. Second item could be a new ticket, I suppose, but it's part of the original scope of the feature, I'd say.

comment:23 in reply to: ↑ 22 lancewillett6 months ago

Replying to celloexpressions:

Remaining items left:

  • Come up with better names for accent_lighter and accent_much_lighter. The variables don't matter much, but those labels are also used for the theme_mods.

I'm fine with the variable names. Let's not worry about it.

  • Look into applying the primary accent color to links and highlight color in TinyMCE.

Can you make a new ticket?

  • Remove the ability to change the "header text color" -- which is really only the site title text color. See #25540

I think it's OK to leave that as-is.

comment:24 lancewillett6 months ago

  • Keywords close removed
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.