WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#31303 closed task (blessed) (fixed)

Add theme-browsing and theme-switching to the Customizer

Reported by: celloexpressions Owned by: markjaquith
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Customize Keywords: has-patch
Focuses: ui Cc:

Description

This ticket is for merging the Customizer Theme Switcher feature-plugin and addressing related issues that require core patches.

See https://make.wordpress.org/core/2015/02/11/customizer-theme-switcher-feature-plugin-merge-proposal/.

Plugin for testing (4.1-compatible): https://wordpress.org/plugins/customizer-theme-switcher/

Attachments (4)

31303.diff (25.4 KB) - added by celloexpressions 3 years ago.
Direct merge of the Customizer Theme Switcher plugin into core, without addressing other issues that couldn't be handled in the plugin.
31303.2.diff (31.0 KB) - added by celloexpressions 3 years ago.
Address issues that required core patches and couldn't be implemented in the plugin.
31303.3.diff (12.7 KB) - added by westonruter 3 years ago.
https://github.com/xwp/wordpress-develop/pull/73/files
phpstorm-jshint-customize-controls.png (129.7 KB) - added by westonruter 3 years ago.
Screenshot of JSHint issue in PHPStorm

Download all attachments as: .zip

Change History (16)

@celloexpressions
3 years ago

Direct merge of the Customizer Theme Switcher plugin into core, without addressing other issues that couldn't be handled in the plugin.

#1 @celloexpressions
3 years ago

  • Component changed from Themes to Customize

Now everyone watching both components has been notified :)

All of the code changes here are on the Customizer side, and are UI-level.

@celloexpressions
3 years ago

Address issues that required core patches and couldn't be implemented in the plugin.

#2 @celloexpressions
3 years ago

31303.2.diff addresses all of the known issues in the plugin that required core patches:

  • Remove #customize-info for theme previews.
  • Change front-end admin bar Themes link to point to themes in the Customizer (deep-linked).
  • When a new theme is activated, go to the home page (front end), not the themes admin.
  • If user doesn't confirm that they want to leave unsaved changes, remove customize-loading body class.

Further enhancements, primarily changing the way the themes header displays, should probably happen after a first-pass is in (see also #31336 and #31289).

#3 @ocean90
3 years ago

  • Milestone changed from Awaiting Review to 4.2

#4 @DrewAPicture
3 years ago

  • Type changed from feature request to task (blessed)

#5 @markjaquith
3 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 31533:

Add theme browsing and theme switching to the Customizer

  • Brings into core the Customizer Theme Switcher feature plugin
  • You can now browse, preview, and activate themes right from the Customizer

fixes #31303.
props celloexpressions, afercia, westonruter, folletto, designsimply

#6 follow-up: @westonruter
3 years ago

Attached 31303.3.diff with some fixes for jshint and some improvements to escaping.

#7 in reply to: ↑ 6 ; follow-up: @ocean90
3 years ago

Replying to westonruter:

Attached 31303.3.diff with some fixes for jshint and some improvements to escaping.

We don't escape translations like that. I also can't see our build tools producing JSHint errors. Where did you get them from?

@westonruter
3 years ago

Screenshot of JSHint issue in PHPStorm

#8 in reply to: ↑ 7 ; follow-ups: @westonruter
3 years ago

Replying to ocean90:

Replying to westonruter:

Attached 31303.3.diff with some fixes for jshint and some improvements to escaping.

We don't escape translations like that.

Why not? Isn't it generally a good idea in the case of malicious POT files? And it is being used once here, in class-wp-customize-section.php:

<h2><?php esc_html_e( 'Themes' ); ?>

I also can't see our build tools producing JSHint errors. Where did you get them from?

Regarding the JSHint errors, see my phpstorm-jshint-customize-controls.png above. Specifically the issue is reported in JSHint within PhpStorm, although it is not reported from the command line JSHint. But actually, I saw that my PhpStrom was using JSHint 2.2. I just updated it to 2.5.10, and it is now not complaining about these lines. Nevertheless, the mixing of tabs and spaces here does not seem ideal.

#9 in reply to: ↑ 8 @dd32
3 years ago

Replying to westonruter:

Replying to ocean90:

Replying to westonruter:

Attached 31303.3.diff with some fixes for jshint and some improvements to escaping.

We don't escape translations like that.

Why not? Isn't it generally a good idea in the case of malicious POT files?

Basically we generally trust the translations, and don't waste the extra time in processing their contents.
There's a lot of strings in WordPress, and unless we're going to change them all, there's little point in changing a few, when you combine that with the fact a string may legitimately have HTML in it, you suddenly get to a point where if a malicious translation wants to affect the screen, it just means it has to target a specific string on the page, rather than the other 20.

#10 in reply to: ↑ 8 ; follow-up: @ocean90
3 years ago

Replying to westonruter:

And it is being used once here, in class-wp-customize-section.php:

<h2><?php esc_html_e( 'Themes' ); ?>

This should be removed, see also #30724.

#11 in reply to: ↑ 10 @celloexpressions
3 years ago

Replying to ocean90:

Replying to westonruter:

And it is being used once here, in class-wp-customize-section.php:

<h2><?php esc_html_e( 'Themes' ); ?>

This should be removed, see also #30724.

Handled in #31289 (which is still waiting for feedback/commit).

#12 @ocean90
3 years ago

In 31892:

Customizer Theme Switcher: Reset font size of theme names in overlay. Apply left position only to themes section.

see #31303.

Note: See TracTickets for help on using tickets.