Skip to content
This repository has been archived by the owner on Feb 17, 2025. It is now read-only.

Blockbase: Use the Global Styles rest API in the customizer #5492

Merged
merged 23 commits into from
Feb 15, 2022

Conversation

matiasbenedetto
Copy link
Member

@matiasbenedetto matiasbenedetto commented Feb 9, 2022

Changes proposed in this Pull Request:

  • Blockbase: Use the Global Styles rest API in the customizer instead of our current approach.
  • Removed Duotone settings saving because there seem to be not available in the customizer
  • Revert this PR: Skatepark: dynamic duotone support #4740

Related issue(s):

#5308

@matiasbenedetto matiasbenedetto linked an issue Feb 9, 2022 that may be closed by this pull request
@scruffian
Copy link
Member

It's bit tricky to review this with the coding standards changes combined. Would you be able to split that into two different commit, or two different PRs? Thansk!

@scruffian
Copy link
Member

This looks great. Just a few suggestion and I think we can merge

@pbking
Copy link
Contributor

pbking commented Feb 14, 2022

This seems to blow away font customizations. I believe that portion will need another look. Steps (with Blockbase):

Using the Customizer:

  • Change the fonts and publish changes. (Reload the customizer to show that they stuck)
  • Change the colors and publish the changes.
  • Reloading the Customizer will show that the color choices remain but the font choices are back to the default values.

I tested the same steps against /trunk to verify that the behavior was expected before this change.

@matiasbenedetto
Copy link
Member Author

matiasbenedetto commented Feb 14, 2022

The last problem described by @pbking seems to be caused by this commit dded23b

The issue is related to the existence in the database of styles saved to "custom" key, example: settings.color.pallette.custom. With the linked commit we are saving the customized options to: settings.color.pallette.theme but they are not applied because it seems that custom has higher priority.

Knowing this: should we add a few lines to handle this issue (move existing user custom styles from custom to the theme key)? or just revert the linked commit and continue using the custom key? What do you think @scruffian @pbking ?

Personally, I think it would be simpler and make sense to continue using the custom path because these are settings set using the customizer but maybe there are reasons to prefer the theme path instead.

I reverted the commit so you can test again @pbking.

@scruffian
Copy link
Member

I think its fine to merge using the custom key and then address that issue separately if we want to.

@pbking
Copy link
Contributor

pbking commented Feb 15, 2022

I concur with @scruffian. This fixes the issue at hand and we can address the "custom" bit in another PR. I removed the reference to this fixing #5448 in the description and I believe this is ready to come in.

Copy link
Contributor

@pbking pbking left a comment

Choose a reason for hiding this comment

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

This LGTM!

@matiasbenedetto matiasbenedetto merged commit 3b0ae93 into trunk Feb 15, 2022
@mikachan mikachan deleted the update/5308 branch February 16, 2022 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blockbase: Use the Global Styles rest API in the customizer
4 participants