Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quadrat: Add color customization #3856

Closed
wants to merge 17 commits into from
Closed

Conversation

scruffian
Copy link
Member

@scruffian scruffian commented May 14, 2021

This is a different approach to #3817.

The general approach is to replicate the functionality of Global Styles in the Customizer. This means that we modify the custom post type, so that the data can be shared between both environments.

To simplify this I have also removed the custom background and custom foreground colors and implemented new variables for these, which can be referenced in the CSS.

I have also added a preview to the Customizer which just injects some CSS to target the relevant selectors and also modifies the variables.

The code is written to be extendable so it should be trivial to add support for custom link colors too.

@scruffian scruffian requested a review from a team May 14, 2021 15:07
@scruffian scruffian self-assigned this May 14, 2021
@pbking
Copy link
Contributor

pbking commented May 14, 2021

I'm not convinced that we should save the background/foreground in custom - I think they should probably write to the same place as they do with Global Styles.

I wholeheartedly agree. That makes those two properties "universal" so it's a solid step.

@pbking
Copy link
Contributor

pbking commented May 14, 2021

Oh, man this PR has been a LOT of fun to tinker with. Thank you so much! A couple of things I now realize:

This CPT IS where the global styles store user data. I see that now and it would be trivial (ish) to pull/push those values from styles.color.text and styles.color.background such that the SAME property is viewable/editable in both the customizer and the FSE. Fantastic.

After tinkering with this I also don't believe that we should be storing any of these customization in theme_mod but instead rely completely on the CPT for storing the user's preference.

The "merging" conversation regarding custom I'm still wrestling with but I'm (currently, at the time of writing) agreeing with you that it seems the theme and user 'custom' blocks should be merged rather than replaced.

@scruffian scruffian changed the title Try/color customization 4 Quadrat: Add color customization May 19, 2021
@scruffian scruffian marked this pull request as ready for review May 19, 2021 15:34
@pbking
Copy link
Contributor

pbking commented May 19, 2021

Currently it seems that custom variables aren't coming through (at least that's my guess)

The site title size isn't correct and all of the button styles have gone away. I suspect there's more and that it's all related to the same something.

image

I thought it might have been from a previous incarnation of user customized theme.json values so to be sure I recreated the environment and have the same result.

From what I can tell it's the call to get_merged_data that's causing the theme custom attributes to barf:

		$all           = WP_Theme_JSON_Resolver::get_merged_data( $settings, 'theme' );

@jffng
Copy link
Contributor

jffng commented May 19, 2021

I can confirm seeing the same as @pbking.

@scruffian
Copy link
Member Author

Thanks for testing. I've now fixed that issue.

@jffng jffng linked an issue May 20, 2021 that may be closed by this pull request
@pbking
Copy link
Contributor

pbking commented May 21, 2021

I've banged around a lot on this. Love the real-time preview. I haven't been able to get it to do anything other than what I expected so in that I think it's pretty solid.

I'm super-tickled to have a mechanism by which to manipulate the user-controlled theme.json! Really appreciate you making this work for real!

To finalize this we should still account for more colors beyond just foreground/background, and that those colors should also be stored in the CPT in the "custom" space. I realize that won't be editable in the FSE but as color management tooling it just not complete for what Quadrat needs.

I've seen some usage of color picking tooling that included custom palette colors. Would it be possible to add that in (and to expand Quadrat's descriptive palette to include some of the alternate colors in the figma)?

/**
* Add Global Styles Variables
*/
require get_template_directory() . '/inc/global-styles-variables.php';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be keen for this (and related) changes (moving foreground/background variable consumption from custom to generated variables) into trunk sooner than later (even without the color changing bits). Definitely a good change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree. I'll make a PR for this.

@jffng
Copy link
Contributor

jffng commented May 21, 2021

I poked around a bit and if you remove this condition from global styles, you get your color customizations in the post editor too!

@scruffian
Copy link
Member Author

I poked around a bit and if you remove this condition from global styles, you get your color customizations in the post editor too!

Is that still necessary if we add a file in block-templates/index.html?

@jffng
Copy link
Contributor

jffng commented May 21, 2021

No but if we add a block-templates/index.html, we get the site editor instead of the customizer, until there is theme support / theme.json setting for disabling it.

array(
'name' => __( 'Foreground Color' ),
'type' => 'color',
'default' => $this->get_color( $theme_json['styles']['color']['text'], $color_palette ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran into an issue:

  1. I change the foreground and background colors on this PR
  2. Switched to a different PR, the color customizations persisted
  3. Switched back to this PR to change the colors back to default
  4. Clicking default in the color picker didn't work as expected — instead it just returned them to whatever custom color they were last set to.

Do you know why that would happen?

@scruffian
Copy link
Member Author

Closing in favour of #4090

@scruffian scruffian closed this Jun 25, 2021
@pbking pbking deleted the try/color-customization-4 branch September 13, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quadrat: customize the theme colors
3 participants