WordPress.org

Make WordPress Core

#23557 closed defect (bug) (fixed)

Twenty Thirteen: fix sidebar overflow: long sidebars overlap footer

Reported by: mercime Owned by: lancewillett
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Per image attached, sidebar - footer areas on category pages, single post pages with comments disabled, some pages, etc.

Attachments (9)

2013-sidebar-footer.png (154.8 KB) - added by mercime 14 months ago.
2013-footer-overflow.png (22.2 KB) - added by mindctrl 14 months ago.
23557.diff (1.5 KB) - added by obenland 13 months ago.
23557.1.diff (1.5 KB) - added by sbrajesh 13 months ago.
23557.2.diff (1.5 KB) - added by obenland 13 months ago.
Cleaned up version of sbrajesh's approach.
2013-sidebar-footer-23557patch.png (177.9 KB) - added by mercime 13 months ago.
23557.diff
2013-sidebar-footer-23557-1n2.png (62.8 KB) - added by mercime 13 months ago.
23557.1.diff and 23557.2.diff
23557.3.diff (1.6 KB) - added by obenland 13 months ago.
23557.4.diff (1.6 KB) - added by obenland 13 months ago.

Download all attachments as: .zip

Change History (47)

comment:1 SergeyBiryukov14 months ago

  • Component changed from General to Bundled Theme

comment:2 follow-up: lancewillett14 months ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 3.6
  • Priority changed from normal to low

This is intentional, as part of the design. Leaving ticket open for now, for further discussion and brainstorming ideas to make this look better.

comment:3 follow-up: bpetty14 months ago

In the Twenty Thirteen announcement it mentions:

"but it supports a sidebar, if you really insist"

I don't see how the theme can "support a sidebar", but you're saying it shouldn't and that this is intended to not work.

The sidebar has still been enabled by default for years, and even if that is changed in core just for this theme in 3.6, there's still millions of blogs with it enabled when they switch to this theme. So while this theme may have been intended to be used without a sidebar, in reality, it's going to be used with a sidebar far more frequently than without (including in a default WP installation if core isn't changed to disabled the sidebar by default).

comment:4 in reply to: ↑ 3 lancewillett14 months ago

Replying to bpetty:

I don't see how the theme can "support a sidebar"

Themes must explicitly support a sidebar. See register_sidebar() -- http://codex.wordpress.org/Function_Reference/register_sidebar. It's an optional theme feature that each theme author can choose to enable if they wish.

comment:5 sabreuse14 months ago

  • Cc sabreuse added

comment:6 in reply to: ↑ 2 srajbr14 months ago

Replying to lancewillett:

This is intentional, as part of the design. Leaving ticket open for now, for further discussion and brainstorming ideas to make this look better.

Hi, I am new to wordpress trac.
From the files, it is purely intentional. But I believe most of the users are going to use sidebar. So, may be it can be made to look like the max-width:999px view (bottom) by default. Its just an option. Or if its really worth to keep the sidebar to right, I guess lots of css changes need to be done. I can help if you want.

comment:7 lancewillett14 months ago

We're very much open to ideas for improving the footer / sidebar clearing.

comment:8 follow-up: sharonaustin14 months ago

  • Type changed from defect (bug) to feature request

Hi, that Twenty Thirteen theme is jaw-dropping gorgeous; one of the most beautiful things I've seen. But if you would, please, reconsider the sidebar issue.

Please don't get rid of the sidebar, or make it difficult for the average user to implement one. As someone who works with institutions who are attempting to use Wordpress as a content management system, not a blog, I personally think much harm would be done for the average Joe to use Wordpress if a determination is made that special steps must be taken to include a sidebar. Please don't do this. Twenty Thirteen is gorgeous, I want it, I love it, but I could not recommend it to others as a CMS if the sidebar is not readily available for the multiple navigation links needed by many agencies.

Again, the theme is absolutely gorgeous. Breathtakingly beautiful. I am always amazed at what the Wordpress team does, always grateful for the determination to make it a platform of the people.

Thanks for listening.

comment:9 in reply to: ↑ 8 aaroncampbell14 months ago

Replying to sharonaustin:

Please don't get rid of the sidebar, or make it difficult for the average user to implement one. As someone who works with institutions who are attempting to use Wordpress as a content management system, not a blog, I personally think much harm would be done for the average Joe to use Wordpress if a determination is made that special steps must be taken to include a sidebar.

The Twenty Thirteen theme is specifically designed for blogs. Twenty Twelve will still be shipping with WordPress and is probably a better starting place (at least in my opinion) for a WordPress-as-a-CMS theme, especially if you're going to be relying heavily on a sidebar.

comment:10 mindctrl14 months ago

Related, but different. If this needs to be on a separate ticket, let me know.

Footer widgets that contain a lot of content overflow at the bottom, too. See attached 2013-footer-overflow.png.

comment:11 lancewillett14 months ago

@mindctrl -- Yes, that's a new ticket.

comment:12 lancewillett14 months ago

  • Milestone 3.6 deleted
  • Status changed from new to closed

Technique discussed today in IRC during office hours, use JavaScript to calculate the height of the sidebar. If it's greater than the main container + footer, change the main container height to match.

comment:13 bpetty14 months ago

Is that planned to go into the theme itself then, or are you just saying that's the technique that anyone who wants this behavior to change on their installation has to use in a child theme to fix it?

Also, if anyone else missed it like I almost did, this ticket was actually really addressed in #23644.

comment:14 lancewillett14 months ago

  • Keywords needs-patch added; dev-feedback removed
  • Milestone set to 3.6
  • Priority changed from low to normal
  • Status changed from closed to reopened
  • Type changed from feature request to defect (bug)

Just an idea for a technique to go into the theme, nothing planned yet.

comment:15 lancewillett14 months ago

I didn't intend to close this earlier.

comment:16 lancewillett14 months ago

@mindctrl Could you re-test the issue you had with the overflow in footer? If it's still occurring, please open a new ticket for us.

comment:17 lancewillett14 months ago

  • Summary changed from Twenty Thirteen: Sidebar overflow to Twenty Thirteen: fix sidebar overflow: long sidebars overlap footer

comment:18 follow-ups: mercime14 months ago

use JavaScript to calculate the height of the sidebar. If it's greater than the main container + footer, change the main container height to match.

I was thinking of a simpler solution such as triggering a body class e.g. "add-secondary-widget", if widget is added to main-widget-area-now-secondary-widget-area via functions.php so that there's a hook to manipulate layout when that secondary widget area is used and no more absolute positioning :-)

Last edited 14 months ago by mercime (previous) (diff)

comment:19 in reply to: ↑ 18 srajbr14 months ago

Replying to mercime:

use JavaScript to calculate the height of the sidebar. If it's greater than the main container + footer, change the main container height to match.

I was thinking of a simpler solution such as triggering a body class e.g. "add-secondary-widget", if widget is added to main-widget-area-now-secondary-widget-area via functions.php so that there's a hook to manipulate layout when that secondary widget area is used and no more absolute positioning :-)

But this will create another problem- for screen width <= 999px, extra space will be added between the post and the sidebar.

comment:20 in reply to: ↑ 18 ; follow-up: lancewillett14 months ago

Replying to mercime:

I was thinking of a simpler solution such as triggering a body class e.g. "add-secondary-widget", if widget is added to main-widget-area-now-secondary-widget-area via functions.php so that there's a hook to manipulate layout when that secondary widget area is used and no more absolute positioning :-)

That already exists, "sidebar" is added to body class.

comment:21 follow-up: markoheijnen14 months ago

Can't we just drop masonry for the footer. We really don't need it there. It can easily be arranged with PHP. It seems we only need to add an additional class every 4th item.

comment:22 in reply to: ↑ 20 mercime14 months ago

Replying to lancewillett:

That already exists, "sidebar" is added to body class.

You're right, of course. Been mulling about this, CSS won't cut it as it'll ruin the intended design. It can be implement via JS. I saw it done before for absolute positioned sidebar but can't recall where. Shades of: you see it when you don't need it, then when you need it, you can't find it :-)

comment:23 in reply to: ↑ 21 aaroncampbell14 months ago

Replying to markoheijnen:

Can't we just drop masonry for the footer. We really don't need it there. It can easily be arranged with PHP. It seems we only need to add an additional class every 4th item.

With masonry:
http://cl.ly/image/0z0D100M0f2X/Screenshot%20from%202013-03-05%2009:41:43.png

Without masonry (but with .site-footer .widget:nth-child(4n){margin-right:0;}):
http://cl.ly/image/261F3y0b2M1h/Screenshot%20from%202013-03-05%2009:43:22.png

I think Masonry goes a long way toward improving the display of the footer widgets when the sizes differ.

comment:24 markoheijnen14 months ago

Ah didn't noticed that it would also drop in. I thought only lines so without masonry it would drop from 4 to 7.

comment:25 follow-up: mindctrl14 months ago

  • Cc mindctrl added

@lancewillet, the footer overflow is no longer happening.

comment:26 in reply to: ↑ 25 lancewillett14 months ago

Replying to mindctrl:

@lancewillet, the footer overflow is no longer happening.

?? Can you please elaborate? Link to a URL, screenshots?

Scratch that. I misunderstood. Thanks. :)

Last edited 14 months ago by lancewillett (previous) (diff)

comment:27 obenland13 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

obenland13 months ago

comment:28 sbrajesh13 months ago

Here is another take on the patch.
we can simply set the min height on the #main div with the max(height of the contained element, i.e the maximum height from the #tertiary or #primary).
The patch takes care of the margin/padding/positioning of the elements.


sbrajesh13 months ago

comment:29 sbrajesh13 months ago

  • Cc sbrajesh added

obenland13 months ago

Cleaned up version of sbrajesh's approach.

comment:30 obenland13 months ago

sbrajesh, can you point me to resources about jQuery's height() problem with absolut positioned elements?
I tested it both with .height() and .outterHeight( true ) and came to the same results.

comment:31 sbrajesh13 months ago

obenland, since we both have used the height/outerHeight on the child of absolutely positioned element, there is no difference.
The height/outerHeight are not recommended to use for the absolutely positioned element(in our case #tertiary). That will give unpredictable result.

http://api.jquery.com/height/

The only difference in my approach was I used outerHeight to account for margin/padding.


mercime13 months ago

23557.diff

comment:32 mercime13 months ago

Thanks obenland and sbrajesh! Tested the patches in Chrome, FF, Safari, IE and Opera.

Attached screenshots for tests made on patches:

23557.diff - #tertiary sidebar overlaps the #secondary sidebar below content but clears #site-info per first screenshot above

23557.1.diff and 23557.2.diff - #tertiary sidebar clears <footer> which contains #secondary sidebar and #site-info per second screenshot. Looking good.

Cheers.

Last edited 13 months ago by mercime (previous) (diff)

comment:33 obenland13 months ago

  • Version set to trunk

Thanks for the side-by-side comparison of the two patches! Let's discuss tomorrow during office hours in which direction we should go.

mercime13 months ago

23557.1.diff and 23557.2.diff

obenland13 months ago

comment:34 lancewillett13 months ago

@obenland Minor in the JS file, but a few parentheses need spacing, and the 0 and 999 checks can be on the right side of comparisons (not using Yoda here since they are not equal signs).

comment:35 follow-up: obenland13 months ago

Can I leave the 0 and 999 checks as they are? I don't see why we should intentionally switch them to non-yoda?

comment:36 in reply to: ↑ 35 lancewillett13 months ago

Replying to obenland:

Can I leave the 0 and 999 checks as they are? I don't see why we should intentionally switch them to non-yoda?

We discussed this at length in Tucson. :)

Yoda is to avoid assignment using = instead of == or ===. In cases of numerical comparison, it doesn't help and is harder to read.

obenland13 months ago

comment:37 lancewillett13 months ago

  • Keywords needs-testing removed

.4 patch tests out well.

In committing this I'm adding one set of parentheses in the tertiary notation on line 16:

secondary = ( 0 == sidebar.length ) ? -40 : sidebar.height(),

for better readability around the comparison statement.

Also updating the adjustFooter comment a tiny bit.

comment:38 lancewillett13 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from reopened to closed

In 23676:

Twenty Thirteen: enable a JavaScript-based fix for long sidebar areas that overflow the footer. Props obenland, fixes #23557.

Note: See TracTickets for help on using tickets.