#39272 closed defect (bug) (fixed)
Twenty Seventeen: Incorrect $content_width
Reported by: | celloexpressions | Owned by: | laurelfulford |
---|---|---|---|
Milestone: | 4.7.1 | Priority: | normal |
Severity: | major | Version: | 4.7 |
Component: | Bundled Theme | Keywords: | has-patch fixed-major |
Focuses: | Cc: |
Description
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.
Attachments (7)
Change History (25)
#1
@
8 years ago
- Keywords has-patch reporter-feedback added; needs-patch removed
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
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#3
@
8 years ago
- Keywords needs-refresh added; has-patch reporter-feedback removed
- Owner set to laurelfulford
- Status changed from new to assigned
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.
#4
@
8 years ago
- Keywords has-patch added; needs-refresh removed
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.
#5
@
8 years ago
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.
#6
@
8 years ago
- Keywords needs-testing added
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.
#7
@
8 years ago
- Keywords dev-feedback added
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?
#8
@
8 years ago
@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.
#9
@
8 years ago
- Keywords dev-feedback removed
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
.
@
8 years ago
Set the default content width at after_setup_theme
. Change twentyseventeen_content_width
function hook to template_redirect
#10
@
8 years ago
- Keywords needs-refresh added
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.
#11
@
8 years ago
- Keywords needs-refresh removed
Hey @davidakennedy, I have updated my patch.
#12
@
8 years ago
- Keywords needs-refresh added; has-patch needs-testing removed
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!
#13
@
8 years ago
- Keywords needs-refresh removed
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!
#16
@
8 years ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
Remove front-page content_width change