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

Remove block-level duotone from skatepark patterns and templates #6884

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Feb 28, 2023

Changes proposed in this Pull Request:

Remove unnecessary block-level duotone filters in skatepark—these filters are already applied globally.

Additionally, it will be easier to use these patterns in other themes since the custom duotone won't be attached.

@@ -27,7 +27,7 @@
<div style="height:40px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:image {"sizeSlug":"large","style":{"color":{"duotone":["#000","#BFF5A5"]}}} -->
<!-- wp:image {"sizeSlug":"large"} -->
Copy link

Choose a reason for hiding this comment

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

I checked the theme.json for skatepark and see core/image is using the default duotone filter which is ["#000","#B9FB9C"]. This image uses a slightly different (more muted) green of #BFF5A5. I'm not sure if that was originally intentional or a mistake. I don't think anyone would notice the change, but I wanted to point it out here to make sure.

Copy link

Choose a reason for hiding this comment

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

I see a precedent for changing from #BFF5A5 to #B9FB9C, and honestly can't imagine someone would visually notice the distinction between the two, so I don't think this should be a blocker.

@jeryj
Copy link

jeryj commented Mar 1, 2023

I used the columns-in-container template to compare trunk to this branch and they present identically on the frontend, but in the editor the duotone from theme doesn't apply. The top image is inserted from trunk template with the manual duotone vs the theme.json applied duotone on the bottom which doesn't show the duotone in the editor. I'm guessing that's a separate issue, but commenting to confirm.

Editor view of skatepark columns in container template

@MaggieCabrera MaggieCabrera requested a review from a team March 3, 2023 10:54
Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

Choose a reason for hiding this comment

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

Yes, I think this is a good change. The bug where the filter is missing on the editor for the patterns is a duotone issue that we should fix in the editor. I will look for the issue and if there is none I'll open one.

@MaggieCabrera
Copy link
Contributor

This is the one! WordPress/gutenberg#48586

@MaggieCabrera MaggieCabrera requested a review from a team March 3, 2023 11:34
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Cool work! LGTM, also noted that issue y'all already caught above with the post editor.

@jffng jffng merged commit abe293b into trunk Mar 3, 2023
@scruffian scruffian deleted the fix/skatepark-pattern-duotone branch March 3, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants