WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#30164 closed enhancement (fixed)

Twenty Fifteen: merge Header & Sidebar text color controls

Reported by: celloexpressions Owned by: iandstewart
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: has-patch commit
Focuses: ui Cc:

Description

We only have one control for the header/sidebar background color and header/sidebar background image, so why do we have separate controls for different parts of the text's colors? In addition to being confusing, we should be able to simplify the interaction with colorschemes a bit, as well as simplifying the user experience by considering "Decisions, not Options".

This should also make it faster/easier to Customize the theme by having one less option, without removing a particularly useful customization option. Patch should clean up all of the related code, although I may have missed a couple things.

Attachments (3)

30164.diff (2.0 KB) - added by celloexpressions 6 years ago.
Remove header text color control, and use the sidebar text color for those elements.
30164.2.diff (5.1 KB) - added by celloexpressions 6 years ago.
30164.3.diff (5.1 KB) - added by iamtakashi 6 years ago.
Updated patch

Download all attachments as: .zip

Change History (14)

@celloexpressions
6 years ago

Remove header text color control, and use the sidebar text color for those elements.

#1 @iamtakashi
6 years ago

I like this approach and we don't need to worry about #29980 but three things.

  • We need to disable "Text Color" option in Appearance -> Header since it won't do anything.
  • Let's change the label for the control in Customizer to "Header & Sidebar Text Color".
  • We need to cleanup the color-scheme-control.js also.

#2 follow-up: @celloexpressions
6 years ago

Appearance -> Header is being deprecated - it'll only be accessible by users with no js, on IE7 or in IE8 or IE9 on multisite with domain mapping (those are the situations when the Customizer isn't available). Users will otherwise be redirected to the appropriate controls in the Customizer So I actually think we shouldn't consider it at all in Twenty Fifteen, although I added it to that file for now.

New patch addresses the other notes. Noting that we need to use the custom sidebar_textcolor control for this, since the core control is tied to the option to hide the header text, and this color applies to more than that.

#3 in reply to: ↑ 2 @iamtakashi
6 years ago

Replying to celloexpressions:

Appearance -> Header is being deprecated - it'll only be accessible by users with no js, on IE7 or in IE8 or IE9 on multisite with domain mapping (those are the situations when the Customizer isn't available). Users will otherwise be redirected to the appropriate controls in the Customizer So I actually think we shouldn't consider it at all in Twenty Fifteen, although I added it to that file for now.

OK, that's good to know. In that case, I think hiding it with CSS is fine too.

New patch addresses the other notes. Noting that we need to use the custom sidebar_textcolor control for this, since the core control is tied to the option to hide the header text, and this color applies to more than that.

Yes. That makes sense.

I've tested 30164.2.diff and I haven't seen anything weird other than the known issues — the escaped label #29572 and the default background color #30125, #30031.

When this is committed, we can close #29980.

#4 @iandstewart
6 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.1

#5 @iandstewart
6 years ago

Looks like this patch is out of sync for me and I _think_ for iamtakashi as well.

#6 @celloexpressions
6 years ago

  • Keywords needs-refresh added

Hmm, I can't apply it anymore either, and it seems to be rejecting applying to any files (and can't find the right paths). If someone has time to do a manual refresh, that's probably needed.

@iamtakashi
6 years ago

Updated patch

#7 @iamtakashi
6 years ago

  • Keywords needs-refresh removed

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


6 years ago

#10 @iandstewart
6 years ago

  • Keywords commit added; needs-testing removed

#11 @iandstewart
6 years ago

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

In 30230:

Twenty Fifteen: Simplify the header, sidebar, background controls and make customization faster for users to do. This has the added benefit of fixing our bug where hidden header text wasn't being updated on color scheme switch. Nice.

Props celloexpressions, iamtakashi, fixes #30164 and #29980.

Note: See TracTickets for help on using tickets.