#19910 closed task (blessed) (fixed)
Appearance Improvements: Theme Customization Frame
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 3.4 | Priority: | normal |
| Severity: | normal | Version: | 3.3.1 |
| Component: | Customize | Keywords: | dev-feedback needs-docs |
| Focuses: | Cc: |
Description
For context, see the appearance umbrella ticket: #19909
Building a frame to house the customization UI and theme preview is the first step to improving the theme customization process. This will be the first iteration for my team (myself and ocean90). Once the frame is in place, we'll add components to the UI one by one (e.g. custom headers, backgrounds, etc).
- Add an iframe that can preview any theme (call this the target theme).
- Add a sidebar to house future UI.
- Temporarily activate the target theme when building the sidebar UI.
- Add an API to communicate with the iframe.
- Pass data to the iframe.
- Refresh the iframe (likely via post).
- Refresh the iframe's CSS (via postMessage).
- Add a temporary UI to demonstrate the API.
- Buttons that refresh and make trivial changes to the iframe.
- Add a save action (that, by default, will do nothing).
Attachments (8)
Change History (236)
#1
@
13 years ago
- Summary changed from Project Gandalf: Theme Customization Frame to Appearance Improvements: Theme Customization Frame
#5
follow-ups:
↓ 6
↓ 7
@
13 years ago
- Cc Myatu added
Please ensure to comment the code more, as this addition will affect several plugins (including mine). Think about remove_custom_background() as well.
#8
follow-ups:
↓ 10
↓ 11
@
13 years ago
Some initial thoughts about [19995]:
- This code can't run on the front-end, wouldn't it be better to be in wp-admin, not in wp-content.
- Why do we need another JS framework on top of jQuery to handle this? It only makes the code harder to read/understand, brings a lot of complexity and a lot of unneeded features (for example how many instances of the classes and subclasses would we need to show a preview?).
- Wouldn't it be better to reduce the number of files? As far as I see customize-loader can be added to an existing JS file and customize-base, customize-controls and customize-preview can be merged into one file. Same with the four new php files and two css files.
#9
follow-ups:
↓ 12
↓ 10
@
13 years ago
With the customizer some functions aren't necessary anymore.
- preview_theme()
- _preview_theme_template_filter()
- _preview_theme_stylesheet_filter()
- preview_theme_ob_filter()
- preview_theme_ob_filter_callback()
What should we do we with these?
I did a quick search through the plugin repo and there is only one plugin, which is using preview_theme_ob_filter directly:
$ ack 'preview_theme' --type=php
ktai-style/inc/theme.php
530:public static function preview_theme() {
550: ob_start( 'preview_theme_ob_filter' );
#10
in reply to:
↑ 8
;
follow-up:
↓ 13
@
13 years ago
Replying to azaozz:
Some initial thoughts about [19995]:
- This code can't run on the front-end, wouldn't it be better to be in wp-admin, not in wp-content.
You mean wp-includes? Also the preview iframe is more or less front end.
Wouldn't it be better to reduce the number of files?
For the beginning I think it's fine, we still can combine them later, if needed.
Why do we need another JS framework on top of jQuery to handle this?
You mean postMessage?
---
"jQuery on front end" – we should keep #19979 in mind.
#11
in reply to:
↑ 8
;
follow-ups:
↓ 14
↓ 12
@
13 years ago
Replying to azaozz:
Some initial thoughts about [19995]:
I handled the merge of [19995] into core (koopersmith advised, then committed), so I can speak to these decisions.
This code can't run on the front-end, wouldn't it be better to be in wp-admin, not in wp-content.
Yes it can. And in fact, it does, due to how the preview iframe works. There is no actual harm to placing JS in wp-includes over wp-admin. The rule of thumb I have used is that if it is a library of sorts, or decoupled nicely from the admin, or something that could be employed on the frontend (like pointers), it should go into wp-includes. The only things that should go into wp-admin is code intrinsically tied to the admin interface.
Why do we need another JS framework on top of jQuery to handle this? It only makes the code harder to read/understand, brings a lot of complexity and a lot of unneeded features (for example how many instances of the classes and subclasses would we need to show a preview?).
There is not another framework here, nor is there a whole lot of complexity. (I was able to understand what was going on.) I cover the breakdown of JS files.
Wouldn't it be better to reduce the number of files? As far as I see customize-loader can be added to an existing JS file and customize-base, customize-controls and customize-preview can be merged into one file. Same with the four new php files and two css files.
For the JS files, no. They are loaded in different places. customize-loader.js handles the ability to load the customizer. customize-controls gets loaded in the frame that handles the controls. customize-preview gets loaded in the preview iframe. customize-base is a shared base class of both customize-preview and customize-controls. As they are nicely decoupled, they should remain separate.
For the PHP files, no. Individual classes belong in separate files. The fourth file, customize-controls.php, is a special file that is included to handle the controls after WP is hijacked.
#12
in reply to:
↑ 9
@
13 years ago
Replying to ocean90:
With the customizer some functions aren't necessary anymore.
- preview_theme()
- _preview_theme_template_filter()
- _preview_theme_stylesheet_filter()
- preview_theme_ob_filter()
- preview_theme_ob_filter_callback()
We should probably leave these alone. While you've found no one referencing most of these functions directly, I would not be surprised if many of them are relying on ?template= &stylesheet= for their own particular needs. It's one of those under-the-hood pieces that we probably shouldn't break.
#13
in reply to:
↑ 10
;
follow-ups:
↓ 14
↓ 15
↓ 16
@
13 years ago
Replying to ocean90:
You mean wp-includes? Also the preview iframe is more or less front end.
Yes, sry, meant wp-includes. No, a preview of the front-end only makes sense when the user is in the admin. So it's logical for the code to be in wp-admin.
... You mean postMessage?
No, I meant customize-base.js. It is build as a JS framework which may be useful if the previews were a stand-alone JS application but don't make sense in the current (WP admin only) scope.
#14
in reply to:
↑ 11
;
follow-up:
↓ 18
@
13 years ago
Replying to nacin:
Yes it can. And in fact, it does, due to how the preview iframe works. There is no actual harm to placing JS in wp-includes over wp-admin. The rule of thumb I have used is that if it is a library of sorts, or decoupled nicely from the admin, or something that could be employed on the frontend (like pointers), it should go into wp-includes. The only things that should go into wp-admin is code intrinsically tied to the admin interface.
Yes, in general there's no harm. I was talking about the php and css code too, not only the JS. Imho it doesn't make sense all these files to be in wp-includes as they all are only used when the user is in the admin and are useless on the front-end.
There is not another framework here, nor is there a whole lot of complexity.
Not exactly. The JS is build as a framework on top of jQuery with the actual functions that "do the work" being several levels "deep". Don't see a point in having JS classes for a few jQuery based single-use lines of JS.
For the JS files, no. They are loaded in different places. customize-loader.js handles the ability to load the customizer. customize-controls gets loaded in the frame that handles the controls. customize-preview gets loaded in the preview iframe. customize-base is a shared base class of both customize-preview and customize-controls. As they are nicely decoupled, they should remain separate.
Agreed. Generally loading one larger JS file is better than segmenting it into several small files as after the first load the browsers keep all of it in memory cache. The same is true for the css. However when using JS concatenation the file that is loaded would probably be already different so the browsers would have to load it again.
For the PHP files, no. Individual classes belong in separate files. The fourth file, customize-controls.php, is a special file that is included to handle the controls after WP is hijacked.
I'm not sure it's a good idea to use OOP structures to output HTML in a non-OOP app. There is only one screen so there is only one "instance" of the HTML. What's the point of building specific OOP structure for only this part of the code that is used only once? But that discussion is not for here.
#15
@
13 years ago
- Removes thickbox enqueue
- Fixes an issue with theme.js, it doesn't require thickbox (see r15656)
- Uses data attributes to pass template/stylesheet value
#16
@
13 years ago
Before anything, I'd like to note that this is a first pass and certain portions are underdeveloped or unfinished. Please take everything with a grain of salt.
Andrew, I hope this addresses a few of your questions.
Why use a class for WP_Customize?
The WP_Customize class does not just include customize-controls.php. It also handles correctly previewing a theme (in both the admin and front-end), registering and maintaining sections and settings, and properly switching the theme and saving settings.
Why is this in wp-includes?
To preview a theme setting, we need to be able to override it on the front end; the code has to run there. On top of that, it would be fairly easy to alter the WP_Customize class to run completely independent of the admin (if we removed the final keyword, that is).
What's going on in customize-base.js?
There is some underlying sugar there for several reasons. The most prominent is not fully operational yet, which is that there will be an option for the preview to apply changes without completely refreshing the page. This option will be facilitated by postMessage and synchronized values. Instead of writing code to individually watch, bind and synchronize each input, the sugar provides a system to keep all of them in sync with only a few lines of code.
It's important to note that this is JS-only UI. One of the main goals here is to make it easy for plugins and themes to add to the customize controls and preview without having to unbind large portions of core code.
#17
follow-up:
↓ 21
@
13 years ago
Just some food for thought...
Looking at how sections and settings are added in the WP_Customize class, wouldn't there be a benefit to add a filter to add_setting and add_section. This provides ample opportunity to filter (modify) the respective setting or sections, without having to resort to a later remove_setting?
And this simply nitpicking: Sections and settings are rendered in the order it is saved in their respective arrays (in WP_Customize). Since these are protected, no outside reference is given so the order cannot be changed (nor is there the ability to do so in add_setting or add_section). This would be helpful to move certain sections or settings up and down the list for specific theme customizations (ie., a "Footer" right after the "Header" section, as "Static Front Page" and "Set Title & Tagline" would have little to do with it - makes it a bit more intuitive for the end-user.
#21
in reply to:
↑ 17
@
13 years ago
Replying to Myatu:
Looking at how sections and settings are added in the
WP_Customizeclass, wouldn't there be a benefit to add a filter toadd_settingandadd_section. This provides ample opportunity to filter (modify) the respective setting or sections, without having to resort to a laterremove_setting?
Settings and sections are objects for just that reason — so that they can be modified without having to be entirely replaced. That said, the add_setting and add_section methods aren't set in stone yet. We'll be moving away from a strict $customize global in favor of dependency injection. I'm not convinced that we'll need a filter there, but let's revisit the idea once things settle a bit.
Replying to Myatu:
And this simply nitpicking: Sections and settings are rendered in the order it is saved in their respective arrays (in
WP_Customize).
Settings and sections both contain priority parameters (you can specify these in the constructor or alter them afterwards) which serve as the primary means of sorting sections before they're rendered. The order settings/sections are added serves as a secondary sorting mechanism (tiebreaker) when multiple items share the same priority.
Thanks for the feedback!
#23
follow-ups:
↓ 24
↓ 24
@
13 years ago
In [20034]:
Theme Customizer: Trigger UI updates only when necessary.
#24
follow-up:
↓ 25
@
13 years ago
Replying to nacin:
It's not being executed, only defined, and it is quite small.
Right. It just adds a tiny bit of memory on every hit to the front-end and a tiny bit on every function_exists() call. There already are quite a few functions in wp-includes that are defined but cannot be used, was only hoping we won't be adding more.
Probably because the handlers for Plupload is not exactly a well-formed API.
Plupload is a relatively "young" JS library and the APIs are changing quite a bit from release to release. Proxying the settings seems less desirable than simply including the settings object as outputted from wp_plupload_default_settings() and letting plugins add the hook names/IDs for their HTML.
Also the current integration for the "main" uploader is mostly hard-coded. Was hoping to move it to the new API but that can wait for 3.5 :)
#25
in reply to:
↑ 24
;
follow-up:
↓ 26
@
13 years ago
Replying to cais:
This may be more observation and be a result of coder error but I found the following to cause the Customization > Navigation to not produce the results you would expect...
I'll look into this. If I'm reading this correctly, is the only difference that the nav menu registration is attached to init instead of after_setup_theme?
#27
follow-ups:
↓ 30
↓ 28
@
13 years ago
Replying to koopersmith:
Replying to cais:
This may be more observation and be a result of coder error but I found the following to cause the Customization > Navigation to not produce the results you would expect...
I'll look into this. If I'm reading this correctly, is the only difference that the nav menu registration is attached to
initinstead ofafter_setup_theme?
No, not quite ... here is a link to the actual functions.php file for Desk Mess Mirrored. It may be easier to see the code in its unadulterated form: https://github.com/Cais/desk-mess-mirrored/blob/master/functions.php
Starting at line 248 is the desk_mess_mirrored_setup function that is used with the 'after_theme_setup' hook, while inside that function starting at line 365 is the 'register_nav_menu' function call which uses the 'init' hook ...
... the more I look at this the more I think using the 'init' hook this way is not correct and could just be awkward code catching up to me. Although I have not seen any issues with this "menu code" since I implemented it back around WP 3.1.
#30
in reply to:
↑ 27
;
follow-ups:
↓ 31
↓ 36
@
13 years ago
Replying to cais:
Starting at line 248 is the
desk_mess_mirrored_setupfunction that is used with the 'after_theme_setup' hook, while inside that function starting at line 365 is the 'register_nav_menu' function call which uses the 'init' hook ...
Nesting function calls in PHP is not very intuitive or easy to read, and since they all end up in global scope, it's functionally the same. Koopersmith is correct when he says "If I'm reading this correctly, is the only difference that the nav menu registration is attached to init instead of after_setup_theme?" That is exactly what ends up happening here.
... the more I look at this the more I think using the 'init' hook this way is not correct and could just be awkward code catching up to me. Although I have not seen any issues with this "menu code" since I implemented it back around WP 3.1.
Correct. init is the wrong hook. after_setup_theme is the only valid hook to use for custom menus, custom backgrounds, custom headers, post thumbnails, etc. The init hook does work in those cases, nonetheless.
[20055] should consider wp_loaded — init should probably be given a chance to finish.
#31
in reply to:
↑ 30
;
follow-up:
↓ 34
@
13 years ago
Replying to nacin:
[20055] should consider wp_loaded — init should probably be given a chance to finish.
This got me thinking... since the customize settings won't be initialized during init, anything that grabs a theme value at that point won't receive the overridden value. That's bad. We need to split up the initialization and make things as JIT as possible to ensure accurately preview-able values.
#34
in reply to:
↑ 31
@
13 years ago
Replying to koopersmith:
Replying to nacin:
[20055] should consider wp_loaded — init should probably be given a chance to finish.
This got me thinking... since the customize settings won't be initialized during init, anything that grabs a theme value at that point won't receive the overridden value. That's bad. We need to split up the initialization and make things as JIT as possible to ensure accurately preview-able values.
Updated to use wp_loaded in [20058]. Nacin, Ryan, and I will revisit this, add_setting, and the like early next week.
#36
in reply to:
↑ 30
@
13 years ago
Replying to nacin:
Replying to cais:
Correct. init is the wrong hook. after_setup_theme is the only valid hook to use for custom menus, custom backgrounds, custom headers, post thumbnails, etc. The init hook does work in those cases, nonetheless.
[20055] should consider wp_loaded — init should probably be given a chance to finish.
Thanks, I'll correct the theme code ... glad this was able to help sort out an issue that had not been addressed, yet.
#37
follow-up:
↓ 27
@
13 years ago
Back button should close Theme Customizer rather than return to a previous page.
#53
@
13 years ago
- Cc drecodeam added
Reported as a new ticket here: #20452
Seeing a strange issue in Opera 11.62 (Opera/9.80 (Macintosh; Intel Mac OS X 10.7.3; U; en) Presto/2.10.229 Version/11.62 ), Clicking links within the preview frame is causing the Customizer to mostly disappear, working fine in Safari however.
The Close button remains, and the frame is still loaded in the background somewhere I think..
Example of what I'm seeing:
#55
follow-ups:
↓ 22
↓ 56
@
13 years ago
Note: I reviewed [20179] with Nacin, Ryan, and Mark in person at SXSW. Feedback welcome.
#22
in reply to:
↑ 55
;
follow-up:
↓ 24
@
13 years ago
Replying to koopersmith:
Note: I reviewed [20179] with Nacin, Ryan, and Mark in person at SXSW. Feedback welcome.
Some quick feedback:
- Moving the settings for plupload to a separate function is nice, well done.
- Having that function defined every time a visitor or a bot hits the front-end is not (this is a part of a bigger problem that eventually will have to be solved: there are a lot of functions in wp-includes that cannot be used on the front-end unless the user is logged in and have certain privileges. 99%+ of the hits on the front-end fall into this category).
- Not sure there is a need for wp-plupload.js at all, at least not in it's current form. Why is it needed to proxy the settings for an external library? Perhaps it would be clearer if wp_plupload_default_settings() actually contained the simple function needed to initialize plupload instead of needing yet another JS file.
#23
follow-ups:
↓ 24
↓ 24
@
13 years ago
Having that function defined every time a visitor or a bot hits the front-end is not
It's not being executed, only defined, and it is quite small.
Not sure there is a need for wp-plupload.js at all, at least not in it's current form. Why is it needed to proxy the settings for an external library?
Probably because the handlers for Plupload is not exactly a well-formed API.
#24
in reply to:
↑ 23
;
follow-up:
↓ 25
@
13 years ago
Replying to nacin:
It's not being executed, only defined, and it is quite small.
Right. It just adds a tiny bit of memory on every hit to the front-end and a tiny bit on every function_exists() call. There already are quite a few functions in wp-includes that are defined but cannot be used, was only hoping we won't be adding more.
Probably because the handlers for Plupload is not exactly a well-formed API.
Plupload is a relatively "young" JS library and the APIs are changing quite a bit from release to release. Proxying the settings seems less desirable than simply including the settings object as defined in wp_plupload_default_settings() and letting plugins add the hook names/IDs for their HTML.
Also the current integration for the "main" uploader is mostly hard-coded. Was hoping to move it to the new API but that can wait for 3.5 :)
#25
follow-up:
↓ 26
@
13 years ago
Right. It just adds a tiny bit of memory on every hit to the front-end and a tiny bit on every function_exists() call. There already are quite a few functions in wp-includes that are defined but cannot be used, was only hoping we won't be adding more.
I don't see a function_exists() call here. But this function *can* be used on the front-end — see also the ajax upload-attachment action.
#26
in reply to:
↑ 25
@
13 years ago
Replying to nacin:
I don't see a function_exists() call here.
What I mean is that every function_exists() call that runs on the front end will have to check one more function name which in PHP is always global.
#27
in reply to:
↑ 37
;
follow-ups:
↓ 30
↓ 28
@
13 years ago
- Cc mike.schroder@… added
Replying to soulseekah:
Back button should close Theme Customizer rather than return to a previous page.
Unsure if this is being worked on, but that has been bothering me as well.
I find myself clicking to preview or customize themes, then clicking the back button in the browser, and ending up on a different page than expected.
#28
in reply to:
↑ 27
@
13 years ago
Replying to DH-Shredder:
Replying to soulseekah:
Back button should close Theme Customizer rather than return to a previous page.
Unsure if this is being worked on, but that has been bothering me as well.
I find myself clicking to preview or customize themes, then clicking the back button in the browser, and ending up on a different page than expected.
Indeed, I've caught myself doing that too. However, I think that the UI should be changed a little so it is clear to the end user (s)he is viewing a modal window over top the Themes screen, akin the existing Theme Preview.
This would also give it more consistency with the current UI, which it is currently lacking. And if screen real estate becomes an issue with a slightly smaller window, provide the option for the end user to expand the modal window to "full screen".
It should be fairly straight forward to implement, given the customization is already inside a div container. Modifying the behaviour of the 'Back' button on the other hand is a technical mess.
#53
@
13 years ago
Seeing a strange issue in Opera 11.62 (Opera/9.80 (Macintosh; Intel Mac OS X 10.7.3; U; en) Presto/2.10.229 Version/11.62 ), Clicking links within the preview frame is causing the Customizer to mostly disappear, working fine in Safari however.
The Close button remains, and the frame is still loaded in the background somewhere I think..
Example of what I'm seeing:
#55
follow-ups:
↓ 22
↓ 56
@
13 years ago
Should browser back button close Theme Customizer rather than return to a previous page?
#56
in reply to:
↑ 55
@
13 years ago
Replying to soulseekah:
Should browser back button close Theme Customizer rather than return to a previous page?
It should return to Install Themes. See #20337.
#63
follow-up:
↓ 69
@
13 years ago
I think we should prevent a direct access to /wp-admin/admin.php?template=twentyten&stylesheet=twentyten&customize=on
#64
@
13 years ago
19910.rtl-support.patch does add RTL support for the customizer.
#66
@
13 years ago
In class WP_Customize_Setting, the preview() function does not pass back the post_value() in the customize_preview_ action, although action in the update() function.
Although I can still obtain the post_value() for the setting by jumping through a few hoops, it doesn't make much sense not to return the value in the customize_preview_ action instead.
#67
follow-up:
↓ 68
@
13 years ago
Looking at the update() function, it uses $this->type opposed to $this->id. So action customize_save_ should be used instead - my bad. Still doesn't pass back the value though :(
#68
in reply to:
↑ 67
@
13 years ago
Replying to Myatu:
Looking at the
update()function, it uses$this->typeopposed to$this->id. So actioncustomize_save_should be used instead - my bad. Still doesn't pass back the value though :(
You're correct — after looking through the actions and filters, a few names/arguments are inconsistent. I'll try to simplify things a bit.
#69
in reply to:
↑ 63
@
13 years ago
Replying to ocean90:
I think we should prevent a direct access to
/wp-admin/admin.php?template=twentyten&stylesheet=twentyten&customize=on
I disagree — having the frame accessible by URL is very useful for debugging and does not significantly restrict or alter the experience.
#6
follow-ups:
↓ 8
↓ 25
@
12 years ago
CSS: Controls in the content column, ie .customize-control-select select make use of fixed widths, which do not account for a scroll-bar that may appear if the expanded content exceeds the height of the browser window.
Also, the scroll bar looks wonky in Chrome (not tested in others as of yet), as shown below (scrolled all the way to the top):
#8
in reply to:
↑ 6
;
follow-ups:
↓ 10
↓ 11
@
12 years ago
Replying to Myatu:
CSS: Controls in the content column, ie
.customize-control-select selectmake use of fixed widths, which do not account for a scroll-bar that may appear if the expanded content exceeds the height of the browser window.
Also, the scroll bar looks wonky in Chrome (not tested in others as of yet), as shown below (scrolled all the way to the top)
Thanks for the report Myatu. I've noticed both of these issues, but haven't been able to address them just yet. Hopefully this'll be fixed up soon — patches welcome. :)
#9
follow-ups:
↓ 12
↓ 10
@
12 years ago
Is there an easy way for code to change the transport value of an existing setting?
For example, I tried to modify a theme to call $sett = $wp_customize->get_setting('blogname'), and then to just set the transport on that object with $sett->transport = 'postMessage'.
However, it gives me back a 500 error. Presumably that's protected somewhere along the line. I can see the value there if I dump it, but attempting to change it = death.
Calling remove_setting and then readding it with the postMessage transport does work, but is obviously less than ideal.
I'm trying to do all this at the customize_register action hook.
#10
in reply to:
↑ 9
;
follow-up:
↓ 13
@
12 years ago
Replying to Otto42:
Is there an easy way for code to change the transport value of an existing setting?
Never mind. I was being stupid. This code works fine when hooked to customize_register:
$sett = $wp_customize->get_setting('blogname');
$sett->transport='postMessage';
@
12 years ago
Patch for twentyeleven to support postMessage updating of title, description, and header text-color
#11
follow-ups:
↓ 14
↓ 12
@
12 years ago
Not sure if that patch belongs in this ticket or not, but it adds some functionality to twentyeleven to make the blog title, description, and header-textcolor use postMessage updating instead of refresh updating. Looks nicer, faster, easier to use. Shows how a theme can use knowledge of itself to make things happen smoother. Might not necessarily be the "right" way.
#12
in reply to:
↑ 11
@
12 years ago
Replying to Otto42:
Not sure if that patch belongs in this ticket or not, but it adds some functionality to twentyeleven to make the blog title, description, and header-textcolor use postMessage updating instead of refresh updating. Looks nicer, faster, easier to use. Shows how a theme can use knowledge of itself to make things happen smoother. Might not necessarily be the "right" way.
Nice. We have #20448 for Twenty Ten and Twenty Eleven.
#14
in reply to:
↑ 13
;
follow-up:
↓ 18
@
12 years ago
- Keywords dev-feedback reporter-feedback needs-docs added
Replying to koopersmith:
In [20649]:
Could you please elaborate on how a theme developer can call functions on WP_Customize instance? How does one instantiate it? I am using a fork of _s' theme and the standard Gandalf features work fine but I cannot find how to add my own.
add_action( 'customize_register' , 'my_customization_function');
function my_customization_function(){
global $wp_customize;
//But $wp_customize is null!
$wp_customize->add_section( 'layout', array(
'title' => __( 'Layout' ),
'priority' => 20,
) );
}
Your help is greatly appreciated!
#16
@
12 years ago
I'm sure you will likely want to make further improvements to the look/feel of the post-install options anyway, but 19910.customizeAfterInstall.diff adds a quote to make the Customize link appear properly after installing a theme.
#18
in reply to:
↑ 14
@
12 years ago
Replying to jackmahoney:
Could you please elaborate on how a theme developer can call functions on WP_Customize instance? How does one instantiate it? I am using a fork of _s' theme and the standard Gandalf features work fine but I cannot find how to add my own.
Your help is greatly appreciated!
jack: I'll be writing a tutorial on this soon, but for the moment some of the implementation details are still being finalized. So anything we document now may be out of date by release.
For the moment, take a look at the patches I've made in Ticket #20448 for the twentyeleven theme. This shows how to implement custom stuff with the current code.
#20
follow-up:
↓ 21
@
12 years ago
- Keywords reporter-feedback removed
We should put all our customize-*.js files into one folder named wp-customizer or something like that. Only for a better overview.
#21
@
12 years ago
Question: Why is this code in the WP_Customize_Setting->preview() function?
case 'option' : if ( empty( $this->id_data[ 'keys' ] ) ) add_filter( 'pre_option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) ); else add_filter( 'option_' . $this->id_data[ 'base' ], array( $this, '_preview_filter' ) ); break;
I mean, I get the case, but I'm not sure of the reason for the diff between the existence of the pre_ vs. non-pre_ filter. If the customizer is just overriding values, shouldn't all of them go through the pre_ filter? I mean, we're not filtering the existing data, we're replacing it. Since the option data is loaded into the customizer in advance, replacing "all" should work, right? Or am I missing a case? Perhaps default cases need to be considered?
#22
follow-up:
↓ 24
@
12 years ago
I understand why the use of pre_option vs. option now (serialized data vs. non-serialized data), however the use of option_ in this case causes the settings preview to not work when the row of settings does not exist in the database at all.
Basically, if the entire option row doesn't exist, then get_option will return the $default value, without applying the option_whatever filter to it, so the preview doesn't reflect the changes made.
I considered a few alternatives, but I think that actually applying the option_$option filter to the default options will cause breakage in some plugins somewhere, and so it's not a viable patch. So instead, I've created a "default_option_$option" filter, which specifically filters only the default options returned. By applying the customizer preview filters to that as well, this problem is fixed.
Potential patch attached.
#23
follow-ups:
↓ 24
↓ 24
@
12 years ago
- Cc chip@… added
For option type settings, that correspond to settings added as an array via register_setting(), is there any way to pass them through the register_setting() $sanitize_callback on-save? Seems that might be more flexible/scalable than requiring a sanitize_callback option for each $wp_customize->add_setting() call.
#24
in reply to:
↑ 23
;
follow-up:
↓ 25
@
12 years ago
Replying to chipbennett:
For
optiontype settings, that correspond to settings added as an array viaregister_setting(), is there any way to pass them through theregister_setting()$sanitize_callbackon-save? Seems that might be more flexible/scalable than requiring asanitize_callbackoption for each$wp_customize->add_setting()call.
Looks like I gave you bad info before, Chip. Options that are added via register_setting *do* get their validation callback ran across them, even when being saved via the theme customizer.
This is because register_setting actually adds the validation function as a filter to sanitize_option_{$option_name}, so regardless of the method by which the option is updated, the sanitize function will still run on its contents.
#25
in reply to:
↑ 6
;
follow-up:
↓ 26
@
12 years ago
Replying to Myatu:
CSS: Controls in the content column, ie
.customize-control-select selectmake use of fixed widths, which do not account for a scroll-bar that may appear if the expanded content exceeds the height of the browser window.
19910.customize-sidebar.diff is a first-pass at addressing that. May need a more thorough sweep through widths in case I missed something.
Also, the scroll bar looks wonky in Chrome (not tested in others as of yet), as shown below (scrolled all the way to the top)
This is because of the header and footer that are absolutely positioned. Not sure what to do (or can be done) about this just yet.
#21
@
12 years ago
- Resolution set to fixed
- Status changed from accepted to closed
Closing this umbrella ticket. Report bugs in new tickets.
#13
follow-ups:
↓ 14
↓ 15
↓ 16
@
12 years ago
If the custom background default wp-head-callback (_custom_background_cb) is used, we use postMessage for all custom background properties. Otherwise, we use full refreshes.
Was looking at this yesterday, didn't suggest a patch since it's a big change. Perhaps something to consider for 3.5 or in the future.
Wouldn't it be simpler and more robust to always do a full refresh of the preview iframe when a user changes something? That would allow WP filters, actions and PHP callbacks to run on the changed value and filter/reject/tweak the value according to the theme and any plugins that may be hooked there or after that.
So instead of the current "flow":
- User changes a setting
- We send the changes back to the server (if they need processing in PHP)
- We receive the filtered values (optional)
- We push them to the preview iframe, applying the changes with JS
we could do:
- User changes a setting
- We send the changes back to the server triggering a refresh of the preview iframe.
In both cases the preview is updated to reflect the new setting. In the second case we don't need to push any changes with JS removing any complications there (cross-domain iframes, etc.).
#15
in reply to:
↑ 13
@
12 years ago
Replying to azaozz:
Wouldn't it be simpler and more robust to always do a full refresh of the preview iframe when a user changes something?
A refresh is very slow by comparison to using postMessage to make instant changes. Seconds instead of under a millisecond. It would be super slow and annoying to have a color change take a few seconds for me to see the effect, when I could just as easily cycle around and see the change in real-time.
As it is now, the theme has the choice of whether to use refresh (default, works for everything) or postMessage (added on, only works for things that the theme can re-implement in JS code).
#16
in reply to:
↑ 13
@
12 years ago
Replying to azaozz:
So instead of the current "flow":
- User changes a setting
- We send the changes back to the server (if they need processing in PHP)
- We receive the filtered values (optional)
- We push them to the preview iframe, applying the changes with JS
Also, as far as I know, your steps two and three there don't happen anywhere. postMessage sends the value directly into the preview frame's JS handler when it changes, it doesn't have the ability to pass it back into the server for filtering, unless I missed something somewhere.
#17
follow-up:
↓ 21
@
12 years ago
Replying to Otto42:
A refresh is very slow by comparison to using postMessage to make instant changes...
Also, as far as I know, your steps two and three there don't happen anywhere...
That's another point: doing "instant" refresh bypasses the possibility to run any PHP hooks and callbacks that may be needed. I know we don't currently filter "background-color" for example but a plugin or theme may decide to filter it. Currently that's not possible to show in the preview.
Regarding speed: right, postMessage is faster than "full" refresh of the preview, however this has it's drawbacks (no IE < 10 when cross-domain, i.e. on most multisite installs current IE won't work). Also speed is somewhat relative, it's "nice to have" but shouldn't prevent running hooks on the user settings.
#18
@
12 years ago
If a theme or plugin is using filters on those relevant pieces in some way, then they have the ability to change the control to be using refresh instead of postMessage as well. It's one line of code:
$wp_customize->get_setting('background-color')->transport='refresh';
As it is, I figure most will be going the other way and changing default refresh settings to be postMessage ones.
Themes we don't need to worry about at all WRT their filters, since the theme author will also have to implement any JS to handle a postMessage method. The controls default to refresh, after all, they have to turn on postMessage.
And yes, I think it should totally prevent hooks running on the settings, for the case where the theme has explicitly said "hey, use postMessage". Not running those through the server is sort of the whole point.
Re: IE, just force it to refresh on all controls when IE and cross-domain is detected. Not much else to do about that. postMessage is an optional add-on that the theme has to both enable and add extra code to support, while refresh is the default.
#20
follow-up:
↓ 21
@
12 years ago
Another (big?) advantage of always doing full refresh of the preview is that the code needed would be a lot simpler and more robust/compatible. For example currently the customizer won't work in FF with the NoScript addon if more strict rules are set for it.
But as I said it's too late to look into this now, perhaps in 3.5.
#21
in reply to:
↑ 20
@
12 years ago
Replying to azaozz:
Another (big?) advantage of always doing full refresh of the preview is that the code needed would be a lot simpler and more robust/compatible. For example currently the customizer won't work in FF with the NoScript addon if more strict rules are set for it.
But as I said it's too late to look into this now, perhaps in 3.5.
I'm confused why this is bring brought up now, seeing that postMessage was part of the original scope and implementation — way back from January/February. Regardless, the user experience is far, far better with postMessage. I'm a backend API person and even then I don't see any kind of justification to do a round-trip. It works, it's simple enough, it's robust, it's compatible. Removing it is a non-starter in my book.
#22
follow-up:
↓ 24
@
12 years ago
Replying to Otto42:
And yes, I think it should totally prevent hooks running on the settings, for the case where the theme has explicitly said "hey, use postMessage". Not running those through the server is sort of the whole point.
So if there's a plugin that runs hooks on a setting and the theme opts for postMessage, the preview would not reflect the filtering done by the plugin and the output will be different from the front-end? The main point of a preview is to be "truthful" :)
Denying PHP hooks on the new settings is a drawback, consider adding menus and widgets to the customizer in the future, "instant" JS driven previews would not be possible for them.
Replying to nacin:
As far as I remember the original scope was to have an API for front-end previews accessible from any page in the admin. Handling the preview from JS only was a "nice-to-have-if-it-works" kind of thing.
The underlying problem is that in some cases the preview won't be "real" without a trip to the server, whether it's an XHR or iframe refresh.
Yes, agree now it's not the time for such discussion, could we leave it for when adding widgets and menus support to the customizer.
#24
in reply to:
↑ 22
;
follow-up:
↓ 25
@
12 years ago
Replying to azaozz:
So if there's a plugin that runs hooks on a setting and the theme opts for postMessage,
In that case the plugin is perfectly capable of overriding the theme's control and changing the transport mode for that control to "refresh" and thus forcing the customizer to refresh the theme.
add_action( 'customize_register', 'force_refresh_demo', 100 ); // late hook to ensure the control is there
function force_refresh_demo($wp_customize) {
wp_customize->get_setting('some-example-setting')->transport='refresh';
}
Voila. Plugin can handle it, and it's the proper place for it to be handled because the plugin knows whether or not it's hooking onto some setting and modifying it in a way that the customizer will need to care about.
Denying PHP hooks on the new settings is a drawback, consider adding menus and widgets to the customizer in the future, "instant" JS driven previews would not be possible for them.
...
The underlying problem is that in some cases the preview won't be "real" without a trip to the server, whether it's an XHR or iframe refresh.
If it is necessary to add some new mechanism later that goes to the server to retrieve some kind of PHP generated data via AJAX, and then pass that into the preview frame, then that can be added later. It doesn't mean that we should dump postMessage and opt for full-refresh all the time, just because some cases don't work well with postMessage. There's nothing wrong with having more than one way to get the job done.
#6
follow-ups:
↓ 8
↓ 25
@
12 years ago
We should say that the text color is only for the header.
See [20912] and 19910.declare-text-color-as-header-text.patch.
#5
follow-ups:
↓ 6
↓ 7
@
11 years ago
Anyone know why the section descriptions were moved out of view and into the title attribute in [20182]? I can't seem to find an explanation.
Development is currently taking place via plugin. Check it out here: https://plugins.svn.wordpress.org/gandalf/branches/dev/