This repository has been archived by the owner. It is now read-only.

Customizer: introduce theme option to disable the image filter #327

Merged
merged 10 commits into from Oct 27, 2018

Conversation

Projects
None yet
7 participants
@eliorivero
Copy link
Contributor

eliorivero commented Oct 25, 2018

Fixes #284

This PR introduces a new section in Customizer with a single option to activate or deactivate the filter applied to featured images.

Customizer control

captura de pantalla 2018-10-27 a la s 13 59 33

Black overlay on single view

Additionally, @kjellr requested in the issue that a black overlay be added to the featured image in single post view. This is how it looks

captura de pantalla 2018-10-25 a la s 14 47 20

Testing instructions

Checkout this branch and build the updated styles with the command line:

npm install && npm run build:style

Now you can go to the Customizer and try the new control.

@eliorivero eliorivero force-pushed the eliorivero:add/image-filter-theme-options branch 3 times, most recently from 7be2942 to aa9ab05 Oct 25, 2018

@eliorivero eliorivero force-pushed the eliorivero:add/image-filter-theme-options branch from aa9ab05 to 48ae1eb Oct 25, 2018

@allancole allancole added this to the RC1 milestone Oct 25, 2018

@allancole allancole self-requested a review Oct 25, 2018

@allancole
Copy link
Collaborator

allancole left a comment

This is looking great @eliorivero! I really like the animation that happens when you turn this off & on.

I’ll pass this off to Kjell, as we’ll probably want to simplify the wording used in the theme option name and description. Otherwise, this is spot on!

@allancole allancole requested a review from kjellr Oct 25, 2018

@LittleBigThing

This comment has been minimized.

Copy link

LittleBigThing commented Oct 25, 2018

Cool! :-)

Maybe a detail: shouldn't the control be a checkbox?
Ideally, the control to select the custom color for the filter should be put below this one and displayed conditionally when this control is active. So, they would need to be in the same section.

@kjellr

This comment has been minimized.

Copy link
Collaborator

kjellr commented Oct 26, 2018

Very cool, thanks @eliorivero! This works well. Here's a GIF of this in action:

image-filter

Just a couple procedural details to figure out...


I have a rough rewrite of the copy:

**Featured Image Color Filter **
By default, Twenty Nineteen adds a duotone-like effect to featured images using your site's primary color. This effect can be turned off. When the color filter is disabled, a standard black overlay will be used on single post pages to preserve readability of the text that sits on top of the featured image.

Featured images should:
[] Have a color filter applied.
[] Not have a color filter applied.

I'd love to see if @michelleweber has some copy thoughts though  — she's always great with this sort of copy. 🙂


shouldn't the control be a checkbox?

I like the radio buttons in this case, because I tend to think of checkboxes as controls that require a second action (like a confirm/submit) button to set them in action. That said, I was just clicking around the Customizer and I can see that the "Display Site Title and Tagline" option is a checkbox that shows its impact immediately. 🙂 I'd like to loop in @melchoyce for a quick checkbox vs. radio button weigh-in too.


Ideally, the control to select the custom color for the filter should be put below this one and displayed conditionally when this control is active. So, they would need to be in the same section.

The custom filter color is actually planned to replace the primary color for your site (basically, everything blue right now: links, buttons, etc.), so it makes more sense to keep that option separate, in the "Colors" section.

@eliorivero eliorivero force-pushed the eliorivero:add/image-filter-theme-options branch from 48ae1eb to a99a5a8 Oct 26, 2018

@eliorivero eliorivero force-pushed the eliorivero:add/image-filter-theme-options branch from a99a5a8 to ff6b7fe Oct 26, 2018

@eliorivero

This comment has been minimized.

Copy link
Contributor Author

eliorivero commented Oct 26, 2018

Thanks for the reviews!

@kjellr I've replaced the copy with the one you provided and updated the screenshot in the original PR description. Minor concern is that this copy now produces an orphan in the paragraph but it's not a huge deal. Removing the "that sits" solves the orphan, not sure if it's acceptable wording

captura de pantalla 2018-10-26 a la s 10 30 52

Will update again if @michelleweber makes changes to the copy.

@kjellr

This comment has been minimized.

Copy link
Collaborator

kjellr commented Oct 26, 2018

Removing the "that sits" solves the orphan, not sure if it's acceptable wording

Thanks @eliorivero — that works. 👍

Will update again if @michelleweber makes changes to the copy.

Actually, I realized Michelle is on vacation. 😄 I think @kristastevens may be able to help though.

@kristastevens

This comment has been minimized.

Copy link

kristastevens commented Oct 26, 2018

Thanks for the ping! I have some suggestions to consider within this one sentence:

Original:

When the color filter is disabled, a standard black overlay will be used on single post pages to preserve readability of the text that sits on top of the featured image.

Proposed:

When the color filter is disabled, a standard black overlay appears on individual post pages to preserve readability of the text on top of the featured image.

@allancole

This comment has been minimized.

Copy link
Collaborator

allancole commented Oct 26, 2018

Ideally, the control to select the custom color for the filter should be put below this one and displayed conditionally when this control is active. So, they would need to be in the same section.

Thought this through a bit and I think @eliorivero had it right here ^^. The Image Filter option is ultimately a decision about color and I think users will have an easier time discovering it if it appears on the same Customizer panel as the Color options. If you add this option to the same panel as #191 and make it appear conditionally, I think we can go ahead and get this in ;-)

@kjellr

This comment has been minimized.

Copy link
Collaborator

kjellr commented Oct 26, 2018

When the color filter is disabled, a standard black overlay appears on individual post pages to preserve readability of the text on top of the featured image.

Thanks, @kristastevens! @eliorivero: let's update that sentence.

If you add this option to the same panel as #191 and make it appear conditionally, I think we can go ahead and get this in ;-)

Sounds good to me too. If this is ready before #191, I suggest we don't worry about the conditional nature yet. We can just move this over to the Colors section and merge it in.

@melchoyce

This comment has been minimized.

Copy link

melchoyce commented Oct 26, 2018

Fun idea!

This is likely scope creep, but what if we did something like:

( O ) Color Filter
[#0000ff] Color Picker
( ) Black Filter

That would make it an option between color or black, rather than something turned on/off. I also think it would be rad to let people choose the color overlay.

@eliorivero

This comment has been minimized.

Copy link
Contributor Author

eliorivero commented Oct 26, 2018

Done, moved control to Colors section and updated last sentence with what Krista suggested.
Updated screenshot in original PR description.

@kjellr

This comment has been minimized.

Copy link
Collaborator

kjellr commented Oct 26, 2018

Looks good, thanks @eliorivero! Can we make one minor change? We should add a "Featured images should:" label above the button options.

( O ) Color Filter
[#0000ff] Color Picker
( ) Black Filter

I do like that, but it'd work only if we let people choose a custom color there in addition to their site's primary color. So let's hold off for now and get this initial functionality in place.

@eliorivero

This comment has been minimized.

Copy link
Contributor Author

eliorivero commented Oct 26, 2018

I've added the label. Please note that we already used the label and the description for this control, so I'm faking it at the end of the description with some CSS. Here's the screenshot (updated in the PR description too) and the previous iteration without the label.

captura de pantalla 2018-10-26 a la s 14 51 18captura de pantalla 2018-10-26 a la s 13 22 23

I could've made the font bigger like the control's real label but I'm afraid to conflict with the typography hierarchy. This is how it looks when the font size is the same than the label. What do you think?

captura de pantalla 2018-10-26 a la s 14 58 40

@kjellr

This comment has been minimized.

Copy link
Collaborator

kjellr commented Oct 26, 2018

Thanks @eliorivero!

Let's go with this for now:

screen shot 2018-10-26 at 2 25 18 pm

(16px top margin instead of 8px, and non-bold text label)

Once that's all set, this should be good to go. 👍

@melchoyce

This comment has been minimized.

Copy link

melchoyce commented Oct 26, 2018

I still think those options are a little confusing — "not have a color filter applied" sounds like you won't necessarily have any filter applied, and I'm sure many people will gloss over the description.

What about:

Featured images will:
Have a color filter applied.
Have a black filter applied.

@kjellr

This comment has been minimized.

Copy link
Collaborator

kjellr commented Oct 26, 2018

Featured images will:
Have a color filter applied.
Have a black filter applied.

@melchoyce You're right, that's more clear. 🙌

@eliorivero, if you can slip that in too that'd be awesome. 🙂

@eliorivero

This comment has been minimized.

Copy link
Contributor Author

eliorivero commented Oct 26, 2018

@melchoyce @kjellr

Featured images will:
Have a color filter applied.
Have a black filter applied.

The second label makes it look like the black filter will be applied everywhere but the featured image doesn't have any color filter in the post list. I think this will be confusing when the user goes to Customizer and deactivates the image filter while being in a post list view, since the featured image doesn't have any color filter there. Perhaps we can add

Featured images will:
Have a color filter applied.
Have a black filter applied only on single post view.

In any case I've preemptively made the change with the new labels suggested

captura de pantalla 2018-10-26 a la s 17 17 44

@kjellr

This comment has been minimized.

Copy link
Collaborator

kjellr commented Oct 27, 2018

The second label makes it look like the black filter will be applied everywhere but the featured image doesn't have any color filter in the post list.

Thanks @eliorivero. I chatted with @benhuberman about that point, and we both felt it's okay, as long as we spell things out clearly in the description. He also had some thoughts on how to trim down and clarify the copy just a bit:

Featured Image Color Filter
Twenty Nineteen adds a color filter to featured images using your site's primary color. If you disable this effect, the theme will use a black filter in individual posts to keep text readable when it appears on top of the featured image.

On Featured Images, apply
A color filter
A black filter

@eliorivero eliorivero force-pushed the eliorivero:add/image-filter-theme-options branch from 194f6a5 to de68584 Oct 27, 2018

@eliorivero

This comment has been minimized.

Copy link
Contributor Author

eliorivero commented Oct 27, 2018

Done, all updated 👍

@kjellr

kjellr approved these changes Oct 27, 2018

Copy link
Collaborator

kjellr left a comment

Looks great! Thanks @eliorivero! 🚢

@kjellr kjellr merged commit 90f8a86 into WordPress:master Oct 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eliorivero

This comment has been minimized.

Copy link
Contributor Author

eliorivero commented Oct 28, 2018

Awesome, thanks for all the reviews and the merge 👍

@eliorivero eliorivero deleted the eliorivero:add/image-filter-theme-options branch Oct 28, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.