Twenty Seventeen: Incorrect $content_width

[celloexpressions]

Twenty Seventeen defines a wider content width on the front page in
twentyseventeen_content_width():

$content_width = 700;

if ( twentyseventeen_is_frontpage() ) {
	$content_width = 1120;
}

But the front page does not use a different content width than other pages. The only case I can think of where it might change is when the one column layout option is used.

Also, I'm not seeing an actual content width of 700px anywhere. Depending on screen size, it looks like it's roughly 520px (+/- 5px).

Incorrectly setting the $content_width causes media embeds to end up with the wrong aspect ratio, potentially among other issues. I'm surprised that this was not picked up sooner; I hadn't looked closely through functions.php but noticed this in passing while working on adding some inline docs there.

[sstoqnov]

Remove front-page content_width change

[sstoqnov]

It seems that the content width check was added so it can be removed the check in front-page.php in the following ticket
​https://github.com/WordPress/twentyseventeen/pull/371/files#diff-78cd5aa3783a74555c9938a2a81d01c6

I checked the front-page.php revisions in github and it seems that this is there from the initial commit.
Not sure why it was added.
​https://github.com/WordPress/twentyseventeen/blob/69482880b4ae2ec145f60df417423665581a5653/front-page.php

[davidakennedy]

Thanks for the ticket @celloexpressions!

I missed this in all my code reviews, and it's something that should be addressed as soon as possible.

Thanks for the patch @sstoqnov, but we'll need to cover more cases than just removing that code.

The widths break down like this:

Static Front Page:
Two Column: 525px (524.31px)
One Column: 644px

Regular Pages:
Two Column: 525px (524.31px)
One Column: 740px

Blog:
Two Column (with sidebar): 525px (524.31px)
One column (no sidebar): 740px

I'm going to assign this to @laurelfulford so she can take a stab at a patch. We can all help test though.

[laurelfulford]

Thanks for catching this @celloexpressions!

39272.1.patch​ builds off the patch @sstoqnov submitted, plus:

• Changes the default $content_width to 525
• Updates the $content_width on the front-page.php to 644px when the one column layout is used.
• Updates the $content_width on pages to 740px when the one column layout is used.
• Updates the $content_width on single posts when no sidebar widgets are assigned.

The blog index, archive and search all show the sidebar widgets as well, but when they're not present it doesn't affect the width of the content area.

[celloexpressions]

Rather than changing it in each template, would it work to keep all of the checks in functions.php with conditional for the possible different sizes? That would be easier to follow and override in child themes.

I'm actually not sure how well the editor handles applying different content widths on different views, but at least the frontend should be correct with the changes either way.

[sstoqnov]

Combine all content_width checks in twentyseventeen_content_width function

[sstoqnov]

Thank for the comment @davidakennedy.

@celloexpressions I agree with you that will be better to have everything in functions.php, but after spending some time I reallized that using after_setup_theme action doesn't allow us to use conditional tags. The previous check that I have removed also doesn't work, because twentyseventeen_is_frontpage always rerun false.

We can use wp hook, so that everything we need will be loaded.

In 39272.2.patch I replaced after_setup_theme with wp action and combine the good work from @laurelfulford in twentyseventeen_content_width function.

[davidakennedy]

Thanks for the work here @laurelfulford and @sstoqnov!

I'd definitely prefer keeping all the content width changes in one function to make it easier to understand and work with for developers.

Some tiny feedback for 39272.2.patch​ @sstoqnov, there are a few comments without spaces after the //. Also, can we end those in a full stop (period)? That's per coding standards we're trying to improve in #39152. :) Otherwise, it's looking good.

In testing, this worked well. I can verify that the after_setup_theme hook isn't sufficient here. From reading the source code, it does seem like the wp hook would be best to use. But I'd like to get a second opinion to be sure. @obenland Is wp the best hook to use here since the code needs to use some conditional tags and theme functions?

[sstoqnov]

Change comments from 39272.2.patch to match WordPress coding standards

[sstoqnov]

@davidakennedy Agree, I have updated my patch to match the coding standards.

Regarding the hook, this was the first one that all conditional tags can be used. I agree that there may have a better one.

[obenland]

In Twenty Thirteen we used template_redirect, wp would work just as well probably.
In any case I'd split this up and define a default value at after_theme_setup so that plugins have something to work with at init.

[sstoqnov]

Set the default content width at after_setup_theme. Change twentyseventeen_content_width function hook to template_redirect

[davidakennedy]

Thanks for the feedback helpful @obenland!

Thanks for the refreshed patch @sstoqnov! It's looking good, and I think this is almost ready. One tiny point of feedback: there are tab spaces on the empty lines in the code. Can you fix those?

Otherwise, this tested well, and I don't see any issues.

[sstoqnov]

Remove tab spaces on empty lines

[sstoqnov]

Hey @davidakennedy, thank you for your comment, I have updated my patch.

[laurelfulford]

Thanks @sstoqnov!

I took a peek and it looks like there are still two tabs on empty lines remaining -- on line 57 and line 206. The others are fixed.

Can you please submit one more patch with these removed -- thank you!

[sstoqnov]

Hey @laurelfulford, thank you for finding this. I am sure that the spaces have been removed in my last patch, but you are right that they still exist. Maybe I have uploaded a wrong page... however a new patch has been uploaded.

Thank you!

[davidakennedy]

In 39635:

Twenty Seventeen: Fix incorrect $content_width value in theme

This addresses a major bug. Incorrectly setting the $content_width causes media embeds to end up with the wrong aspect ratio, among other issues. This fix uses template_redirect, to ensure conditional theme tags can be used. It also defines a default value at after_theme_setup so that plugins have something to work with at init.

Props sstoqnov, laurelfulford, obenland.

Fixes #39272.

[dd32]

In 39650:

Twenty Seventeen: Fix incorrect $content_width value in theme.

This addresses a major bug. Incorrectly setting the $content_width causes media embeds to end up with the wrong aspect ratio, among other issues. This fix uses template_redirect, to ensure conditional theme tags can be used. It also defines a default value at after_theme_setup so that plugins have something to work with at init.

Props sstoqnov, laurelfulford, obenland.
Merges [39635] to the 4.7 branch.
Fixes #39272.