Make WordPress Core

Opened 14 years ago

Closed 12 years ago

#22834 closed enhancement (duplicate)

Live Preview should map sidebars as done on activation

Reported by: alexvorn2 Owned by:
Priority: normal Milestone:
Component: Themes Version: 3.4
Severity: normal Keywords: close
Cc: Focuses:

Description

Problem:

Sidebar disappears in the live preview after theme deactivated.

Reproduce:

Fresh install of the WordPress trac:

  1. Activate Twenty Ten theme
  2. Click the Live Preview of Twenty Twelve
  3. No sidebar in the preview

Bug 100%

Attachments (1)

22834.diff​ (993 bytes ) - added by rmarks 13 years ago.
Based on the logic in comment:5, here is a patch. It has a logic error in that the registering of the sidebars occurs too late.

Download all attachments as: .zip

Change History (17)

#1 @alexvorn2
14 years ago

  • Keywords needs-patch 2nd-opinion added

Expectation:

The sidebar should show with all default widgets in Live Preview

#2 @nacin
14 years ago

  • Component Themes → Appearance
  • Summary Twenty Twelve: Sidebar disappears on Live Preview → Live Preview should map sidebars as done on activation
  • Type defect (bug) → enhancement
  • Version → 3.4

Twenty Ten names its first widget area "primary-widget-area". Twenty Eleven and Twenty Twelve use the more generic/standard "sidebar-1".

Since we do re-map sidebars on activation, we should probably do that on live preview as well.

I have a question. If this is a "Bug 100%" why do you need a second opinion on it? :-)

#3 @alexvorn2
14 years ago

  • Keywords 2nd-opinion removed

because I thought this is a theme bug but actually this is a WP bug, so that's why I needed for someone's opinion.

#4 @alexvorn2
14 years ago

so this going to WP 3.6?

#5 follow-ups: @rmarks
13 years ago

  • Cc rmarks@… added
  • Keywords dev-feedback added

I have looked over possibilities to fix this and think I'm close, but I need a little feedback please. These changes are in /wp-includes/class-wp-customize-manager.php:

In start_previewing_theme(), I propose to add near all of the add_filter()'s
update_option( 'theme_switched', $this->theme->stylesheet );

In stop_previewing_theme(), I propose to add
update_option( 'theme_switched', false );

I tested this code, but when I browse to /wp-admin/customize.php?theme=twentytwelve the first time, I do not see the sidebars. When I refresh, I do see the sidebars. I think I have the first update_option() call in the wrong place. Am I on the right track? Can someone point me toward where this should be?

#6 in reply to: ↑ 5 ; follow-up: @DrewAPicture
13 years ago

Replying to rmarks:

I have looked over possibilities to fix this and think I'm close, but I need a little feedback please.

Would you like to ​submit a patch?

#7 in reply to: ↑ 5 @alexvorn2
13 years ago

Replying to rmarks:

I tested this code, but when I browse to /wp-admin/customize.php?theme=twentytwelve the first time, I do not see the sidebars. When I refresh, I do see the sidebars. I think I have the first update_option() call in the wrong place. Am I on the right track? Can someone point me toward where this should be?

You do not see the sidebars from the first time because they did not registered yet.
I have not looked in the code but I think a dev should look into this because here things are not so easy.

@rmarks
13 years ago

Based on the logic in comment:5, here is a patch. It has a logic error in that the registering of the sidebars occurs too late.

#8 in reply to: ↑ 6 @rmarks
13 years ago

Replying to DrewAPicture:

Would you like to ​submit a patch?

Just did. Thank you for the help yesterday.

#9 follow-up: @DrewAPicture
13 years ago

  • Keywords has-patch added; needs-patch removed

#10 in reply to: ↑ 9 ; follow-up: @alexvorn2
13 years ago

  • Keywords needs-patch added; has-patch removed

Replying to DrewAPicture:
this is not a working patch, please read the comment...
It's just an approach or so.

#11 in reply to: ↑ 10 ; follow-up: @DrewAPicture
13 years ago

  • Cc xoodrew@… added

Replying to alexvorn2:

Replying to DrewAPicture:
this is not a working patch, please read the comment...
It's just an approach or so.

Look, @rmarks is looking for feedback on his approach. There is nothing that says uploaded patches even have to work, but something is better than nothing.

We are here to foster learning in addition to improving core through contribution so if I might make a suggestion, a little less time spent on shooting down suggestions might go a long way toward finding a solution this ticket.

There are such things as incremental patching and learning while doing, which @rmarks seems ready and willing to try. Have a better suggestion? Make a better patch.

#12 in reply to: ↑ 11 @alexvorn2
13 years ago

I think there is need for a better patch, but as I said "I think a dev should look into this because here things are not so easy."

#13 @alexvorn2
13 years ago

  • Component Appearance → Themes
  • Keywords dev-feedback needs-patch removed

Tested the patch, seems to be working for me.

#15 @westonruter
12 years ago

  • Keywords close added
  • Milestone Awaiting Review → 3.9.1

@alexvorn2: I believe that this issue would have been resolved by #27767 (and #27897), please test in 4.0 Alpha (will be in the 3.9.1 release). We're now invoking the widget remapping logic when first loading the Customizer in a theme switch preview. When the theme gets activated, the remapped widgets go live.

#16 @nacin
12 years ago

  • Milestone 3.9.1
  • Resolution → duplicate
  • Status new → closed

Duplicate of #27897.

Note: See TracTickets for help on using tickets.