WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 4 weeks ago

#45928 reviewing enhancement

Twenty Nineteen: Add a filter to the $primary_color variable

Reported by: allancole Owned by: desrosj
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch needs-testing needs-refresh
Focuses: Cc:
PR Number:

Description

There should be a filter added to the $primary_color variable, so that child-themes and plugins can override the built-in default primary color if they choose. Currently, saturation, lightness, and selected text-color can all be filtered. Once the primary color is also filterable, it’ll allow for child themes and plugins to fully customize the color dynamics in the theme.

See: https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentynineteen/inc/color-patterns.php#L15

Attachments (6)

45928.patch (624 bytes) - added by allancole 10 months ago.
45928.1.patch (5.0 KB) - added by mattwiebe 9 months ago.
45928.2.patch (9.4 KB) - added by allancole 9 months ago.
45928.3.patch (9.0 KB) - added by laurelfulford 7 months ago.
45928.4.patch (9.1 KB) - added by laurelfulford 7 months ago.
45928.5.diff (10.1 KB) - added by davidbaumwald 5 weeks ago.
Refreshed Patch

Download all attachments as: .zip

Change History (27)

@allancole
10 months ago

#1 @laurelfulford
10 months ago

  • Component changed from General to Bundled Theme
  • Milestone changed from Awaiting Review to 5.1.1

#2 @LittleBigThing
9 months ago

Don't we need to filter the default primary color (hue) for a couple more instances?

The default value of 199 is used:

@mattwiebe
9 months ago

#3 @mattwiebe
9 months ago

I added 45928.1.patch that will allow for filtering the default hue by a child theme. All hardcoded instances of 199 are replaced with a function call to twentynineteen_get_default_hue().

I also truncated the conditional atop twentynineteen_colors_css_wrap() because it's hooked to wp_head and will never appear anywhere other than the frontend, including within a customizer view.

@allancole
9 months ago

#4 @allancole
9 months ago

Just added 45928.2.patch that expands on @mattwiebe’s filters solution for Hue, and adds a few additional filters to fully support HSL including, Saturation, Lightness, Saturation (text) Selection, Lightness (text) Selection and Lightness Hover colors.

#5 @laurelfulford
8 months ago

Thanks @allancole!

I've tested 45928.2.patch , and it seems to work well for filtering colours in child themes.

One thing I notice is if you're moving from the default colour in the Customizer to a custom colour, the current patch will not preview -- it's throwing a JavaScript error on line 29 of customize-preview.js:

Uncaught TypeError: Cannot read property 'split' of undefined

The custom colour does work fine on the front-end, and if you're moving from one custom colour to another in the Customizer, it previews fine.

This looks like it has to do with the conditional changes inside of twentynineteen_colors_css_wrap() -- with it, the inline CSS the JavaScript is looking for doesn't exist when you start with the default colour.

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


8 months ago

#7 @SergeyBiryukov
8 months ago

  • Summary changed from Twenty Nineteen: Add a filter to the $primay_color variable to Twenty Nineteen: Add a filter to the $primary_color variable

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


8 months ago

#9 @lukecarbis
8 months ago

  • Milestone changed from 5.1.1 to 5.2

#10 @desrosj
7 months ago

@laurelfulford @allancole 5.2 beta 1 is in a little under 2 days. Are either of you able to wrap this up by then? If not, let's punt.

#11 @laurelfulford
7 months ago

Thanks for the reminder on this one, @desrosj!

45928.3.patch fixes the issue I noticed in the previous patch, with the JavaScript error in the Customizer.

Could you please take a look and see if things look correct to you? Thanks!

#12 @laurelfulford
7 months ago

Updated in 45928.4.patch to fix a jshint error about _TwentyNineteenPreviewData not being defined.

#13 @desrosj
7 months ago

  • Milestone changed from 5.2 to 5.3

I did not have time to test this one. The 5.2 beta deadline is just about here. Going to punt this one.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


8 weeks ago

#15 @davidbaumwald
8 weeks ago

  • Keywords needs-refresh added
  • Owner set to abdullahramzan
  • Status changed from new to assigned

#16 @desrosj
5 weeks ago

@abdullahramzan Are you still able to work on refreshing this one?

@davidbaumwald
5 weeks ago

Refreshed Patch

#17 @davidbaumwald
5 weeks ago

  • Keywords needs-refresh removed

This ticket was mentioned in Slack in #core by ianbelanger. View the logs.


5 weeks ago

#19 @desrosj
5 weeks ago

  • Owner changed from abdullahramzan to desrosj
  • Status changed from assigned to reviewing

#20 @desrosj
5 weeks ago

  • Keywords needs-refresh added

I have some questions on this.

If we are introducing functions to get the default hue, saturation, lightness, etc., is there a need for the filters being introduced in the functions.php file? $default_hue is being set by calling one of those *_get_default_* functions, but the saturation, lightness, and lightness hover are not.

Any new filter being introduces should also be accompanied by a proper docblock.

#21 @desrosj
4 weeks ago

  • Milestone changed from 5.3 to 5.4

This one still needs work. With 5.3 beta 1 today, this one needs to be punted.

Note: See TracTickets for help on using tickets.