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.

Sign up for GitHub

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

Jump to bottom

Skatepark: dynamic duotone support #4740

Merged
merged 20 commits into from
Oct 29, 2021
Merged

Skatepark: dynamic duotone support #4740

merged 20 commits into from
Oct 29, 2021

Conversation

MaggieCabrera
Copy link
Contributor

@MaggieCabrera MaggieCabrera commented Sep 29, 2021 •
edited

Changes proposed in this Pull Request:

This PR applies the changes from WordPress/gutenberg#34667 to Skatepark. I have removed the duotone markup from block patterns and templates and defined it directly on theme.json for the affected blocks.

To test this, check that all the images in Skatepark show the duotone filter. Try inserting new ones too :D I went ahead and removed some markup issues from block patterns, check that none of the blocks throw any errors too.

TODO: we need to implement the change to the default filter when the user selects a new color palette on the customizer.

Closes #4411 #4329

@MaggieCabrera MaggieCabrera self-assigned this Sep 29, 2021
@MaggieCabrera MaggieCabrera added the [Theme] Skatepark Automatically generated label for Skatepark. label Sep 29, 2021
@MaggieCabrera MaggieCabrera mentioned this pull request Sep 29, 2021
Merged
@MaggieCabrera MaggieCabrera requested a review from a team September 29, 2021 15:37
@scruffian
Copy link
Member

we need to implement the change to the default filter when the user selects a new color palette on the customizer.

Should we...

  1. Update the definition of the default filter in the theme.json

or

  1. Create a new custom duotone definition and update the theme.json to point to the new custom duotone definition?

I think option 1 makes sense, but I thought it was worth discussing :)

@MaggieCabrera
Copy link
Contributor Author

Should we...

1. Update the definition of the default filter in the theme.json

or

1. Create a new custom duotone definition and update the theme.json to point to the new custom duotone definition?

I think option 1 makes sense, but I thought it was worth discussing :)

Option 1 makes more sense to me because the number of blocks where the filter may be used can vary in the future and we'd have to keep maintaining it or look for every instance of the filter all over the json.

If we change the default filter though we would be losing the green option on the filter menu for the users to chose and duplicating one of the other options. Maybe we should define the green-black filter and the default one, so we don't lose the green one but we'd still always have a duplicate one. Now I'm not so sure!

@ajlende ajlende mentioned this pull request Oct 4, 2021
Closed
@MaggieCabrera
Copy link
Contributor Author

We should consider if we want to add an option to disable duotone from the customizer as a default option for all the blocks.

@scruffian scruffian mentioned this pull request Oct 14, 2021
Open
@scruffian
Copy link
Member

I added a commit to add a updated default filter to the user CPT for Global Styles. This revealed a bug in the way Duotone works: WordPress/gutenberg#35650

I think we're going to get blocked by this :/

@MaggieCabrera
Copy link
Contributor Author

I added a commit to add a updated default filter to the user CPT for Global Styles. This revealed a bug in the way Duotone works: WordPress/gutenberg#35650

I think we're going to get blocked by this :/

We could try option 2, instead of changing the default filter, we just assign a different one to the blocks that need it. It will also help with adding an option to turn duotone off altogether.

@MaggieCabrera MaggieCabrera linked an issue Oct 15, 2021 that may be closed by this pull request
Add User-Configurable Duotone option to Customizer #4434
Closed
@MaggieCabrera
Copy link
Contributor Author

I've been thinking about this and option 2 is not a good idea because it wouldn't work with custom colors anyway, just with the preset palettes from the theme :(

@scruffian
Copy link
Member

I've been thinking about this and option 2 is not a good idea because it wouldn't work with custom colors anyway, just with the preset palettes from the theme :(

What about an option 3 where we create a new filter?

@MaggieCabrera
Copy link
Contributor Author

I've been thinking about this and option 2 is not a good idea because it wouldn't work with custom colors anyway, just with the preset palettes from the theme :(

What about an option 3 where we create a new filter?

We should pair on this, I think!

@MaggieCabrera MaggieCabrera removed their assignment Oct 20, 2021
@MaggieCabrera
Copy link
Contributor Author

Sorry in advance because this code is probably horrible, but it actually works in the frontend! 🎉

Screenshot 2021-10-21 at 18 01 32

What's left to do:

  • Make the preview work. We couldn't figure out how, since we don't really know which selectors we need to target for a given theme. Maybe we just need to force a reload?
  • The way this works now we are replacing the theme's duotone palettes with the user selected one. That is mostly ok but ideally we would merge that with the theme ones.
  • This is assuming that both primary and background colors are defined, we may need to double check that those exist. Maybe not all themes want to use those two as the duotone combination, but that's out of the scope of this PR..

Thanks @mikachan for your help with this!

@MaggieCabrera
Copy link
Contributor Author

Also, I don't think this PR works for testing the GB PR anymore, sorry!

@MaggieCabrera MaggieCabrera self-assigned this Oct 26, 2021
scruffian
scruffian reviewed Oct 26, 2021
View reviewed changes
blockbase/inc/customizer/wp-customize-colors-preview.js Outdated
const colorValues = getValuesFromColors( colors );

if ( document.querySelector( '#wp-duotone-default-filter' ) ) {
updateDuotoneFilter( 'wp-duotone-default-filter', colorValues );
Copy link
Member

Choose a reason for hiding this comment

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

It's a little tricky to understand why there is no hash on this string but there is on line above. It might be better to just move the hash here for consistency.

scruffian
scruffian reviewed Oct 26, 2021
View reviewed changes
blockbase/inc/customizer/wp-customize-colors-preview.js Outdated
if ( userColorDuotone ) {
let colors = palette.map( ( paletteItem ) => paletteItem.color );
//we are inverting the order when we have a darker background so that duotone looks good.
colors = colors.sort( ( first, second ) => {
Copy link
Member

Choose a reason for hiding this comment

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

We can write this in one line

scruffian
scruffian reviewed Oct 26, 2021
View reviewed changes
blockbase/inc/customizer/wp-customize-colors-preview.js Outdated
} );
const colorValues = getValuesFromColors( colors );

if ( document.querySelector( '#wp-duotone-default-filter' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

We could move this check inside the function

@scruffian
Copy link
Member

This is looking good, but I don't see all the filters in the editor:
Screenshot 2021-10-26 at 21 49 51

@MaggieCabrera
Copy link
Contributor Author

This is looking good, but I don't see all the filters in the editor: Screenshot 2021-10-26 at 21 49 51

I tried to merge the user's custom filter with theme preset ones and it's mostly working but in some cases some of the colors are not saving correctly if you var_dump the result of the array_merge from my commit. I can't seem to find what's causing that!

scruffian
scruffian approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM. Don't forget to update the changelog. This should probably be a new minor version!

MaggieCabrera and others added 20 commits October 29, 2021 08:30
@MaggieCabrera @scruffian
added duotone support for blocks, removed it from the patterns markup
46cff43
@scruffian
Add duotone to site logo too
ef05cb8
@scruffian
If the user changes colors via the customizer also update the default…
d85c633 
… Duotone filter
@MaggieCabrera @mikachan @scruffian
created a new filter for the custom duotone
8da85d6 
Co-authored-by: Sarah Norris <sarah@sekai.co.uk>
@MaggieCabrera @scruffian
delete new transient
8f0add1
@MaggieCabrera @scruffian
fix customizer preview and invert darker palettes
c89d3a2
@MaggieCabrera @scruffian
check luminescence on php to see if we need to invert before saving
f483473
@scruffian
Refactoring the code to make it easier to understand
6073d92
@scruffian
Remove closure
ed127f0
@scruffian
refactor for readability
d760ab2
@MaggieCabrera @scruffian
merge user filter with theme ones
7ed11e8
@scruffian
Whitespace
e8221d8
@scruffian
Check the array keys exist properly
57b2519
@scruffian
remove var_dumps
b4c154f
@MaggieCabrera @scruffian
check for the correct values when we define the custom filter
241ded9
@MaggieCabrera @scruffian
minified colord file
ccb0c8b
@MaggieCabrera @scruffian
updated changelog and credited the colord library
53f623c
@MaggieCabrera @scruffian
minified version of colord.js
2cea15a
@MaggieCabrera @scruffian
removed filter from logo
e125c9b
@MaggieCabrera @scruffian
Removed duplicated comment
c78c7e2
@scruffian scruffian merged commit f1a3133 into trunk Oct 29, 2021
@scruffian scruffian deleted the skatepark-duotone-dynamic branch October 29, 2021 07:31
@MaggieCabrera MaggieCabrera removed a link to an issue Nov 3, 2021
Add User-Configurable Duotone option to Customizer #4434
Closed
@pbking pbking mentioned this pull request Nov 10, 2021
Merged
@matiasbenedetto matiasbenedetto mentioned this pull request Feb 11, 2022
Merged
matiasbenedetto added a commit that referenced this pull request Feb 11, 2022
@matiasbenedetto
Revert "Skatepark: dynamic duotone support (#4740)"
5361868 
This reverts commit f1a3133.
scruffian pushed a commit that referenced this pull request Feb 14, 2022
@matiasbenedetto @scruffian
Revert "Skatepark: dynamic duotone support (#4740)"
898ca83 
This reverts commit f1a3133.
matiasbenedetto added a commit that referenced this pull request Feb 15, 2022
@matiasbenedetto @scruffian
Blockbase: Use the Global Styles rest API in the customizer (#5492)
3b0ae93 
* Blockbase: Use the Global Styles rest API in the customizer

* Color and Fonts customizer: changing implementation

* Color and Fonts customizer: changing implementation

* unlinting

* unlinting

* Customizer:  handling when settings or styles are not arrays

* Revert "Skatepark: dynamic duotone support (#4740)"

This reverts commit f1a3133.

* Blockbase: Use the Global Styles rest API in the customizer

* Color and Fonts customizer: changing implementation

* unlinting

* unlinting

* Customizer:  handling when settings or styles are not arrays

* Revert "Skatepark: dynamic duotone support (#4740)"

This reverts commit f1a3133.

* remove unused function

* move settings to the theme key

* revert skatepark changes

* revert skatepark changes

* Renaming variables

* Reverting dded23b

* re-adding delete_transient

* Remonving condition to unset fonts when default is selected

Co-authored-by: Ben Dwyer <ben@scruffian.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scruffian scruffian scruffian approved these changes

Assignees

@MaggieCabrera MaggieCabrera

Labels
[Theme] Skatepark Automatically generated label for Skatepark.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Skatepark: Apply duotone effect to Featured Images by default
2 participants
@MaggieCabrera @scruffian