#30737 closed enhancement (fixed)
JS templates for Customizer Panels and Sections
Reported by: | celloexpressions | Owned by: | westonruter |
---|---|---|---|
Milestone: | 4.3 | Priority: | normal |
Severity: | normal | Version: | 4.1 |
Component: | Customize | Keywords: | has-patch commit |
Focuses: | Cc: | ||
PR Number: |
Description
Follow up to #28907 and #29572. This is necessary for truly dynamic containers, which don't require any ajax and can be added using the new JS API.
This is required for Menu Customizer, which is currently working around the issue by supplying its own template for a generic section. I'd like to get this done early in 4.2.
Working on a patch that's very similar to #29572, but for sections and panels. It will be possible to override in subclasses, of course, like the existing render/render content methods.
If anyone's wondering why we would ever need dynamically-rendered panels, think things like in-page theme switching, where available options change inline based on the previewed theme.
Attachments (19)
Change History (75)
@
5 years ago
Implement support for JS templates for sections and panel contents. Note that panels are like controls, with separate templates/logic for containers vs. content, but sections just have containers. The section part of this patch represents how we'd implement container templates for controls and panels in #30741.
#2
follow-up:
↓ 4
@
5 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from Future Release to 4.2
Would like to get this reviewed, polished, and hopefully commited sooner rather than later so that we can do #30741 and the other related tickets, as well as leveraging all of this in the Menu Customizer project.
This ticket was mentioned in Slack in #core-customize by nacin. View the logs.
5 years ago
#4
in reply to:
↑ 2
@
5 years ago
Replying to celloexpressions:
Would like to get this reviewed, polished, and hopefully commited sooner rather than later so that we can do #30741 and the other related tickets, as well as leveraging all of this in the Menu Customizer project.
I've added feedback for your patch here: https://github.com/xwp/wordpress-develop/pull/59
We also need to update the unit tests to make use of these new methods.
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
5 years ago
#6
@
5 years ago
- Keywords needs-refresh added
Will update to address westonruter's feedback hopefully tonight.
#8
follow-up:
↓ 9
@
5 years ago
- The indentation in class-wp-customize-panel.php and class-wp-customize-section.php is still mixed up.
- render_template() in class-wp-customize-panel.php uses
var classes
for the CSS classes while class-wp-customize-section.php doesn't. The extra variable isn't needed IMO.
#9
in reply to:
↑ 8
@
5 years ago
Replying to ocean90:
- The indentation in class-wp-customize-panel.php and class-wp-customize-section.php is still mixed up.
- render_template() in class-wp-customize-panel.php uses
var classes
for the CSS classes while class-wp-customize-section.php doesn't. The extra variable isn't needed IMO.
#10
@
5 years ago
What about adding to the QUnit tests to check that the new methods work as expected?
Nevermind that we currently don't have any PHPUnit tests at all for WP_Customize_Manager
or WP_Customize_Control
;-P
Also, in the case where a panel/section/control's check_capabilities()
returns false
resulting in the json()
method returning null
, how will this affect _wpCustomizeSettings
in customize.php
which is passed into the JS boot logic? There will be panels/sections/controls in that are null
.
From looking at customize-controls.js
, it seems it would try to pass null
into the constructor, and it would even throw a Null is not an object
error when attempting api.panelConstructor[ data.type ]
. See https://github.com/xwp/wordpress-develop/blob/2b4099c6bd5faa4b99436ab404279699ffc442f9/src/wp-admin/js/customize-controls.js#L2023-L2055
Should the logic instead be moved to customize.php
, for instance:
// Prepare Customize Control objects to pass to JavaScript. foreach ( $wp_customize->controls() as $id => $control ) { if ( $control->check_capabilities() ) { // this conditional added $settings['controls'][ $id ] = $control->json(); } }
Really, however, that whole blob of logic in customize.php
should be moved to a new method on WP_Customize_Manager
, like WP_Customize_Manager::get_configuration()
This ticket was mentioned in Slack in #core by drew. View the logs.
5 years ago
#12
@
5 years ago
I don't think we should hold this up for unit tests since most of the Customizer code still needs unit tests. But if you're able to write some for this, it wouldn't hurt (that's not within my scope of abilities).
I think it's important to keep objects from potentially being sent to JS if the caps aren't available within the control/panel/section objects, but it would probably work if that was handled by the manager. We should probably move all of that logic out of customize.php
in another ticket, so should we just move the checks there for now?
#13
@
5 years ago
Not already having unit tests does not a compelling argument make for skipping them for new functionality.
#14
@
5 years ago
I think it would be great to expose the Container
class to the js API. Because otherwise extending the Section
or Panel
classes with a custom one is a bit problematic. It would be an easy change here: https://github.com/xwp/wordpress-develop/blob/customize-2.0/src/wp-admin/js/customize-controls.js#L227
Anyway thanks for your great work on the Customizer API.
#15
@
5 years ago
If we're going to wait for someone to write unit tests, then this is in a holding pattern indefinitely. I think it's okay without them, but I don't do them so I don't really know. If we need unit tests, we're probably going to have to punt, which has significant implications on the timeline of things like menus in the Customizer happening in core.
@
5 years ago
Additional changes: https://github.com/xwp/wordpress-develop/compare/e07a0f0...3758798
#16
@
5 years ago
In 30737.4.diff:
- Remove
check_capabilities()
calls fromjson()
methods since redundant withprepare_controls()
- Wrap setting exporting with cap checks.
- Add cap checks to panels, sections, and controls before adding to JS exports.
PR: https://github.com/xwp/wordpress-develop/pull/59/files
@celloexpressions: I'll try to add the unit tests over the next 48 hours.
@
5 years ago
Additional changes: https://github.com/xwp/wordpress-develop/compare/de06ebd...55c18ea
#17
@
5 years ago
In 30737.5.diff, I add initial PHPUnit tests for panels.
This ticket was mentioned in Slack in #core-customize by ocean90. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by johnbillion. View the logs.
5 years ago
#20
@
5 years ago
- Keywords needs-unit-tests added
- Milestone changed from 4.2 to Future Release
- Priority changed from high to normal
Punted from 4.2 per bug scrub. This needs some QUnit tests.
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
5 years ago
#23
@
5 years ago
- Keywords needs-refresh added
- Milestone changed from Future Release to 4.3
New menus in Menu Customizer will require users to close the Customizer to go back to the menus panel until we can get this ticket fixed as of #31336. #31336 will also require the actual JS templates for panels and sections to be rewritten, which I can take care of as soon as the first-pass is committed there, along with a general refresh. In the meantime, we need unit tests still, so if anyone can work on those that would be great.
We also need to do this as a first step for #30741, which should also be in 4.3 if at all possible, but that won't happen if we don't get moving here soon.
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
5 years ago
#30
@
5 years ago
- Keywords needs-unit-tests removed
OK, I've added all of the outstanding unit tests, both PHP and JS: 30737.9.diff
@
5 years ago
Add compatibility with #31336: https://github.com/xwp/wordpress-develop/commit/3b6920938ea71fbaadf38eebd005cd79895f843d
#31
@
5 years ago
Refreshed patch in 30737.11.diff after merge of #31336. Includes some resolved conflicts. Needs verification. See pull request as well: https://github.com/xwp/wordpress-develop/pull/87
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
5 years ago
#34
@
5 years ago
Those will need to be fixed ASAP, but I have no experience with QUnit so I'm not the one to fix it. I'll test this patch now that #31336 has landed.
#35
@
5 years ago
With 30737.12.diff I get two PHPUnit failures:
There was 1 error: 1) Tests_WP_Customize_Section::test_json Trying to get property of non-object /srv/www/wp-develop/svn/src/wp-includes/class-wp-customize-section.php:233 /srv/www/wp-develop/svn/tests/phpunit/tests/customize/section.php:121 -- There was 1 failure: 1) Tests_WP_Customize_Panel::test_print_templates_standard Failed asserting that ' <script type="text/html" id="tmpl-customize-panel-default-content"> <li class="panel-meta customize-info accordion-section <# if ( ! data.description ) { #> cannot-expand<# } #>"> <button class="customize-panel-back" tabindex="-1"><span class="screen-reader-text">Back</span></button> <div class="accordion-section-title"> <span class="preview-notice">You are customizing <strong class="panel-title">{{ data.title }}</strong></span> <button class="customize-help-toggle dashicons dashicons-editor-help" tabindex="0" aria-expanded="false"><span class="screen-reader-text">Help</span></button> </div> <# if ( data.description ) { #> <div class="description customize-panel-description"> {{{ data.description }}} </div> <# } #> </li> </script> <script type="text/html" id="tmpl-customize-panel-default"> <li id="accordion-panel-{{ data.id }}" class="accordion-section control-section control-panel control-panel-{{ data.type }}"> <h3 class="accordion-section-title" tabindex="0"> {{ data.title }} <span class="screen-reader-text">Press return or enter to open this panel</span> </h3> <ul class="accordion-sub-container control-panel-content"></ul> </li> </script> ' contains "accordion-section-content". /srv/www/wp-develop/svn/tests/phpunit/tests/customize/panel.php:192 FAILURES!
@
5 years ago
Fix phpunit tests: https://github.com/xwp/wordpress-develop/compare/f3fcf05...c3f5434
#36
@
5 years ago
@celloexpressions I noticed something that may be unexpected when testing with Menu Customizer. Namely, I had to make sure I added:
$this->manager->register_section_type( 'WP_Customize_Menu_Section' );
Or else the custom sections would never render. Nevertheless, Menu Customizer just needed the default section to render right? So it seems that if a custom section didn't have register_section_type
called, it should fallback to render the default section.
So I'm suggesting an additional patch something like this:
-
src/wp-admin/js/customize-controls.js
170 170 container.params = {}; 171 171 $.extend( container, options || {} ); 172 172 container.templateSelector = 'customize-' + container.containerType + '-' + container.params.type; 173 if ( ! document.getElementById( 'tmpl-' + container.templateSelector ) ) { 174 container.templateSelector = 'customize-' + container.containerType + '-default'; 175 } 173 176 container.container = $( container.params.content ); 174 177 if ( 0 === container.container.length ) { 175 178 container.container = $( container.getContainer() ); … … 1211 1214 */ 1212 1215 renderContent: function () { 1213 1216 var template, 1217 templateSelector, 1214 1218 panel = this; 1215 1219 1220 if ( document.getElementById( 'tmpl-' + panel.templateSelector + '-content' ) ) { 1221 templateSelector = panel.templateSelector + '-content'; 1222 } else { 1223 templateSelector = 'customize-' + panel.containerType + '-default-content'; 1224 } 1225 1216 1226 // Add the content to the container. 1217 if ( 0 !== $( '#tmpl-' + panel.templateSelector + '-content').length ) {1218 template = wp.template( panel.templateSelector + '-content');1227 if ( 0 !== $( '#tmpl-' + templateSelector ).length ) { 1228 template = wp.template( templateSelector ); 1219 1229 if ( template && panel.container ) { 1220 1230 panel.container.find( '.accordion-sub-container' ).html( template( panel.params ) ); 1221 1231 }
Thoughts?
#38
@
5 years ago
- Keywords commit added
I'm not finding any UI or behavior changes. There was some expected breakage in Menu Customizer, but beyond that 30737.14.diff looks good. Both QUnit & PHPUnit are passing. Though there is some discussion around jshint
complaints in Slack, once those are fixed I move for commit. Cheers!
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
5 years ago
#42
@
5 years ago
Only two things remain before closing this out:
1) Address the question I've raised regarding falling back to the templates for default
types.
2) Provide a set of default params when constructing new Section
and Panel
objects (and even Controls). As discussed in Slack, if you fail to supply a param.type
, then it will not even fall back to the default
template, but will try to use a undefined
template, which is even worse. So something like this would be good, to go along with my suggestion above.
-
src/wp-admin/js/customize-controls.js
167 167 initialize: function ( id, options ) { 168 168 var container = this; 169 169 container.id = id; 170 container.params = {}; 171 $.extend( container, options || {} ); 170 options = options || {}; 171 options.params = $.extend( 172 { 173 'type': 'default', 174 customizeAction: api.l10n.defaultSectionCustomizeAction 175 // ... 176 }, 177 options.params || {} 178 ); 179 $.extend( container, options ); 172 180 container.templateSelector = 'customize-' + container.containerType + '-' + container.params.type; 173 181 container.container = $( container.params.content ); 174 182 if ( 0 === container.container.length ) {
#43
@
5 years ago
30737.fix-custom-sections.diff fixes issue 1) above. This removes the need for any modifications in Menu Customizer in order for its custom sections to work as they should.
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
5 years ago
#46
@
5 years ago
In 30737.addendum.diff I add QUnit tests for the template fallbacks, and then I also add default params for panels and sections when they are constructed, along with documenting the parameters that are accepted by their constructors.
#47
@
5 years ago
30737.addendum.2.diff moves defaultParams
out of api.Container.initialize
to a defaults
property and adds a defaults
property to api.Section
. I think this looks a bit cleaner since api.Container
shouldn't care about child objects (api.Section
).
#48
follow-up:
↓ 50
@
5 years ago
Looks good to me. Only thing - can we set the action to something better by default? Since that is required for the UI to look right now. Would it be feasible to pass "Customizing > " to JS and then get the parent panel title if it exists?
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
5 years ago
#50
in reply to:
↑ 48
@
5 years ago
Replying to celloexpressions:
Only thing - can we set the action to something better by default? Since that is required for the UI to look right now. Would it be feasible to pass "Customizing > " to JS and then get the parent panel title if it exists?
I'm not a fan of adding a default string here. The UI needs to handle views without an action, for example the Themes "panel" shows an action, which is wrong.
Related: #30741.