Make WordPress Core

Opened 11 years ago

Closed 10 years ago

#22834 closed enhancement (duplicate)

Live Preview should map sidebars as done on activation

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

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 11 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
11 years ago

  • Keywords needs-patch 2nd-opinion added

Expectation:

The sidebar should show with all default widgets in Live Preview

#2 @nacin
11 years ago

  • Component changed from Themes to Appearance
  • Summary changed from Twenty Twelve: Sidebar disappears on Live Preview to Live Preview should map sidebars as done on activation
  • Type changed from defect (bug) to enhancement
  • Version set to 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
11 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
11 years ago

so this going to WP 3.6?

#5 follow-ups: @rmarks
11 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
11 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
11 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
11 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
11 years ago

Replying to DrewAPicture:

Would you like to submit a patch?

Just did. Thank you for the help yesterday.

#9 follow-up: @DrewAPicture
11 years ago

  • Keywords has-patch added; needs-patch removed

#10 in reply to: ↑ 9 ; follow-up: @alexvorn2
11 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
11 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
11 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
10 years ago

  • Component changed from Appearance to Themes
  • Keywords dev-feedback needs-patch removed

Tested the patch, seems to be working for me.

#15 @westonruter
10 years ago

  • Keywords close added
  • Milestone changed from Awaiting Review to 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
10 years ago

  • Milestone 3.9.1 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #27897.

Note: See TracTickets for help on using tickets.