Make WordPress Core

Opened 5 years ago

Closed 7 weeks ago

#45928 closed enhancement (wontfix)

Twenty Nineteen: Add a filter to the $primary_color variable

Reported by: allancole's profile allancole Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch needs-testing needs-refresh close
Focuses: Cc:

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 5 years ago.
45928.1.patch (5.0 KB) - added by mattwiebe 5 years ago.
45928.2.patch (9.4 KB) - added by allancole 5 years ago.
45928.3.patch (9.0 KB) - added by laurelfulford 5 years ago.
45928.4.patch (9.1 KB) - added by laurelfulford 5 years ago.
45928.5.diff (10.1 KB) - added by davidbaumwald 5 years ago.
Refreshed Patch

Download all attachments as: .zip

Change History (32)

@allancole
5 years ago

#1 @laurelfulford
5 years ago

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

#2 @LittleBigThing
5 years 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
5 years ago

#3 @mattwiebe
5 years 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
5 years ago

#4 @allancole
5 years 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
5 years 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.


5 years ago

#7 @SergeyBiryukov
5 years 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.


5 years ago

#9 @lukecarbis
5 years ago

  • Milestone changed from 5.1.1 to 5.2

#10 @desrosj
5 years 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
5 years 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
5 years ago

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

#13 @desrosj
5 years 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.


5 years ago

#15 @davidbaumwald
5 years ago

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

#16 @desrosj
5 years ago

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

@davidbaumwald
5 years ago

Refreshed Patch

#17 @davidbaumwald
5 years ago

  • Keywords needs-refresh removed

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


5 years ago

#19 @desrosj
5 years ago

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

#20 @desrosj
5 years 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
5 years 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.

#22 @davidbaumwald
4 years ago

@desrosj Is this still doable for 5.4 with Beta 1 in a few days?

#23 @ianbelanger
4 years ago

  • Milestone changed from 5.4 to Future Release

@davidbaumwald I'm going to punt this one out of 5.4 because it still does need more work and I don't think we can have it ready for the Beta release.

#24 @desrosj
4 years ago

  • Owner desrosj deleted

#25 @karmatosed
7 weeks ago

  • Keywords close added

I am going to recommend we close this for the following reasons:

  • Initially it was being worked on as part of the default theme's release.
  • Today we would approach colors differently and this isn't causing unless I am mistaken significant user issues today.

Now, if we can find evidence this needs to be resolved for users today let's revisit this. Until then we can consider it something to close as won't fix. Thank you everyone for your collaboration on this so far.

#26 @karmatosed
7 weeks ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

I am going to close this as it has had time for feedback, thank you everyone.

Note: See TracTickets for help on using tickets.