-
Notifications
You must be signed in to change notification settings - Fork 357
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
blockbase checkbox for duotone #4948
Conversation
01e7460
to
09469c3
Compare
This is looking great, just a few small comments. |
Works great for me locally. Wasn't working as expected on wpcom. May be due to the cacheing issue. If so suggest we just park this until that has been fixed and shipped. Not sure how to determine that that is the problem though. |
yes, you can reproduce the problem locally! you just need to remove the dev env variables that are circumventing the cache in the first place. We definitely need the fix before we launch the theme, but with it, the problem goes away (at least for me, locally) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks @MaggieCabrera
yes, you can reproduce the problem locally! you just need to remove the dev env variables that are circumventing the cache in the first place. We definitely need the fix before we launch the theme, but with it, the problem goes away
Confirmed the problem exists and is fixed by the linked issue. Approving these changes to ship once that lands.
8e39db1
to
ab35d71
Compare
I had to add a change because I had JS errors on Blockbase with the original PR, care to have a quick look again @scruffian? |
var enabledDuotone = duotoneVars[ 'duotoneControl' ] === '1' ? true : false; | ||
var enabledDuotone = false; | ||
|
||
if ( window.duotoneVars ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be window.duotoneVars? Couldn't it just be duotoneVars
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it didn't work that way!
Uncaught ReferenceError: duotoneVars is not defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to check the type of duotoneVars:
if ( typeof duotoneVars !== 'undefined' ) {
but I suppose your way works too
I think we should rebase this and check if it still works and then deploy to .org! |
ab35d71
to
ecb7b02
Compare
ecb7b02
to
050ee86
Compare
This needs WordPress/gutenberg#38055 to be merged before it will work. |
050ee86
to
1a2b059
Compare
I rebased this and gave it another test and it still works. If someone else could confirm we can bring it in! |
I don't know if I'll have time today, but did you check dotcom? last time I tried it didn't work in there |
1a2b059
to
813d146
Compare
It's hard to test on wpcom because the Gutenberg fix isn't there, but the customizer preview is working there. |
I just checked on edge sites on dotcom. Seems 12.5 just landed there. However the ability to enable/disable the duotone still didn't seem to be working. It does work locally as described. |
The fix only landed in GB today so it won't be in 12.5 |
I think we should close this after we discussed this |
Do not Merge
Can be merged once this issue has been brought in and has landed on wpcom : WordPress/gutenberg#36236
Changes proposed in this Pull Request:
This PR adds a checkbox for themes that have duotone support that allows the user to turn it off for any blocks that have it set in theme.json directly from the customizer.
There are two known issues with this approach:
EDIT: the cache issue is solved by Fix duotone theme cache WordPress/gutenberg#36236
Related issue(s):
Closes #4434