#29988 closed enhancement (fixed)
Twenty Fifteen: Use JS/postMessage to update the color scheme instead of triggering a page refresh
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 4.1 | Priority: | normal |
| Severity: | normal | Version: | 4.1 |
| Component: | Bundled Theme | Keywords: | has-patch commit |
| Focuses: | javascript | Cc: |
Description
It takes a really long time... :(
Attachments (8)
Change History (53)
#2
@
10 years ago
Even if you replace and generate the *whole* style tag with JS, it'd be a lot faster. :)
#3
@
10 years ago
We really shouldn't do that. The only good way to implement postMessage support here would be for core to support partial preview refreshes: #27355. Otherwise we have to have the colorscheme logic (mostly CSS) duplicated in JS, which makes it even harder to keep in sync with the stylesheet. The general rule of thumb for colorscheme-type options in the Customizer right now is that you have two options:
- Use color pickers, generate css from that in the
<head>, and use onlyrefreshpreviewing. - Offer a pre-defined list of colorshemes (via a select or radio control), and trigger the different schemes with a body class, building the actual styles into a stylesheet that grabs one by class. Then, you can implement
postMessagepretty cleanly by just changing that body class. This is also the best way to implement layout options such as left/right sidebar, and is the fastest way to live-preview leveraging browser capabilities.
Twenty Fifteen does both UI-wise, which could work, although I don't think the UX is very good right now. I'll most likely be making a ticket to clean up the approach for the colorschemes more generally, as it has some major issues based on my initial quick look at it.
But for this, unless we want to significantly change how the colorschemes work, getting #27355 patched and into core in 4.1 is probably our only option here. I had planned on doing it for 4.1, but it got pushed out of priority so that we could focus on things like media in the Customizer, since that was an advertised feature of Twenty Fifteen that we wanted to highlight with revamped background images and the removal of the old admin screens. We can probably make that happen only if we get some breathing room with beta on that ticket (ie, it probably needs a bit more than 2 weeks).
#4
follow-up:
↓ 5
@
10 years ago
Why would you take a trip to the server just for this? Even a partial refresh would be slow. If duplicating the CSS is the problem, then add it with PHP and parse it as a template?
#5
in reply to:
↑ 4
@
10 years ago
Replying to avryl:
Why would you take a trip to the server just for this? Even a partial refresh would be slow. If duplicating the CSS is the problem, then add it with PHP and parse it as a template?
Try justifying doing something like that in a bundled theme to nacin :)
One of the things with bundled themes is that they should be reasonably straightforward, and as simple as possible for new "developers" to dig into the code. And we should also be doing it in the best-practice way. When it comes down to it, duplicating code in PHP and JS is not a good idea. Parsing the php as a template might be good, but if we were to do that we would want to have some sort of a core API to facilitate it.
In terms of a core API that helps developers leverage postMessage in the Customizer more easily/without duplicating code, partial preview refreshes are the best solution we've come up with that should work for most theme/plugin options that have a chance of working without doing a full preview refresh. We've been tossing that idea around for nearly a year now, and have yet to come up with anything better or as accessible and compatible with different scenarios. It comes down to that trip up to the server to grab a bit of output being significantly faster than a complete loading of the front-end of the site as well as images, etc., even if it isn't as instant as a full-JS solution.
For Twenty Fifteen, this also goes back to the two different UIs for colorschemes. When working with color pickers, the delay is fine, and maybe even better as it can be overly distracting (see the background color control). When working with a selector there's a straightforward best-practice way to implement postMessage support. With the combination, we don't have as many options there. Also, looking at the way the CSS is currently implemented, I doubt that it would convert well to JS, considering that it's pretty messy in PHP. I also have some suggestions for accessibility improvements that would increase the code complexity there, thinking along the lines of how Fourteen Colors works; that would almost certainly prevent a php-template-type solution from working.
#6
@
10 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
In 29988.patch I've implemented partial preview refreshes for updating the inline styles when one of their constituent settings is changed. Changing a setting in the Customizer pane causes the setting to get copied to the preview via postMessage and then the preview does an Ajax request to regenerate the CSS and then swaps it out. This is much much faster than refreshing the entire page.
Other changes/fixes made in the course of this patchL
- Core:
styleelements generated bywp_add_inline_stylenow include an ID attribute. This allows them to be selected with JS. - Core: The Customizer Color Picker control was failing to update its state when its underlying setting was changed (sync fail). This has been fixed.
- TwentyFifteen custom backgrounds have been moved to a separate includes file, similar to the custom header includes file.
- TwentyFifteen
functions.php: This is a bit unsightly, but the custom header and custom background callbacks get removed from being added to thewp_headand instead get added towp_enqueue_scripts. This eliminates a priority problem, and it works better sincewp_add_inline_style()is now being used uniformly for all inline style output. - TwentyFifteen
colorSchemecontrol: The color picker element was being found by traversing the DOM, but there are no guarantees that the control is going to remain where the theme expects. So now it is using the properwp.customize.control(…).containerAPI to grab the control's element.
Also on GitHub: https://github.com/xwpco/wordpress-develop/pull/31/files
Regarding a general framework partial-preview refreshes (#27355), I think more use cases need to be fleshed out and patterns need to emerge. Implementing a refresh for CSS in a bundled theme is a nice way to get this idea out there.
This ticket was mentioned in IRC in #wordpress-dev by westonruter. View the logs.
10 years ago
#8
@
10 years ago
We've come up with a plan to add partial preview refreshes to core (#27355). See IRC log. Once patch is added to #27355, then the patch attached here will need to be updated to make use of the new API. This will eliminate all the complexity currently in the patch, though there are a couple things in the patch that should still be added to twentyfifteen and core. I should to open separate tickets for them, I reckon.
#9
@
10 years ago
I've extracted the general Customizer improvements, including the core ColorControl change and the fixes to the Twenty Fifteen colorScheme control, from the above patch into a separate ticket #30031.
#10
@
10 years ago
Also extracted the addition of an id attribute on inline style elements to a separate ticket #30032.
This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.
10 years ago
This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
10 years ago
This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by iandstewart. View the logs.
10 years ago
#17
@
10 years ago
Here's a concept I've used in a couple projects. It uses an Underscore template for the CSS so styles can be generated in the Customizer and passed to the preview.
It doesn't require any trips back to the server, is better for performance on the frontend since the CSS has already been generated, and the template itself is easier to read with named variables (they could be a little more semantic).
Any thoughts on this approach?
#18
@
10 years ago
@bradyvercher: Your approach is ideal. There should be no need for any “partialRefresh” just to apply some PHP data to a template to then inject into the Customizer preview. So using JS templates to dynamically build these stylesheets on the client is definitely the way to go.
I don't see in your patch, however, that the template is being used for rendering out the template in PHP as well? That's the sticky point I think when trying to do JS templating and keeping things DRY: there is no language-agnostic template language in Core, like Mustache. Your Underscore template looks like a strict subset of the syntax available, so it would be straightforward to implement a Underscore.js template subset parser in PHP and re-use the template for rendering out the stylesheet outside the context of the Customizer.
The PHP template processor could be as simple as:
<?php $underscore_tpl = '/* ... */'; $tpl_vars = array( /* ... */ ); $search_replace = array(); foreach ( $tpl_vars as $key => $value ) { $search_replace[ "{{ data.$key }}" ] = $value; } $stylesheet = str_replace( array_keys( $search_replace ), array_values( $search_replace ), $underscore_tpl );
#19
@
10 years ago
I'm generally a big fan of using underscore template for stuff like this as well. Core has a new API for rendering Customizer controls from templates - I wonder if it would be worth doing something similar for setting handling. I could imagine it working similarly, and ideally could all be defined within the PHP file where you add settings (likely as a callback).
#20
@
10 years ago
The difference in the new templates for Customizer controls is that it is building Underscore.js templates to be evaluated by JS dynamically. What we need for Twenty Fifteen is to evaluate the Underscore(ish) templates in both PHP (server-side) for normal site visitors, and then in JS (client-side) when previewing changes in the Customizer.
Has Mustache.php ever been considered for inclusion in Core? I don't see hardly any reference to it in Trac. See also [22415]. There could be a common subset of Mustache/Underscore templates where only interpolation is used, and execution blocks are not allowed. Or the logic-less Mustache syntax for control structures (conditionals and loops) could be translated into the syntax used by wp.template (Underscore.js). If we're already using Underscore.js for templating, it seems like a bad idea to include Mustache.js in Core as well. But on the PHP side, there is no templating language other than PHP itself, if it can be called that. And it is definitely not logic-less or in any way can it be a common subset with JS templates.
#21
follow-up:
↓ 22
@
10 years ago
Maybe I'm missing something obvious, but if the compiled CSS is being saved as a theme mod to serve up to normal site visitors, why does the template need to be parsed with PHP in this case?
#22
in reply to:
↑ 21
;
follow-up:
↓ 26
@
10 years ago
Replying to bradyvercher:
… the compiled CSS is being saved as a theme mod to serve up to normal site visitors …
Probably because it's a bit hidden in your patch. :)
customizer.php:229 and color-scheme-control.js:50-77 are the relevant parts for the theme mod.
#23
@
10 years ago
OK, I took a closer look. I think I missed that critical piece of the color_scheme_css theme_mod setting being added and populated via JS. In that case, there is no need for PHP to render as long as the color_scheme, background_color, and header_background_color, and other settings are only ever modified via the Customizer. Nice one, bradyvercher!
One caution: if the user were able to change one of those settings that comprise the color_scheme_css setting outside the Customizer, for instance in the Appearance > Background admin page, then the JS-computed color_scheme_css would no longer be valid, right? We can help prevent that if #25569 and #25571 can land.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
10 years ago
#25
@
10 years ago
If we go this JS route (which again, is preferred), then we can punt #27355 to 4.2
#26
in reply to:
↑ 22
@
10 years ago
Replying to ocean90:
Probably because it's a bit hidden in your patch. :)
Sorry, didn't mean to make that hidden!
Replying to westonruter
Nice one, bradyvercher!
One caution: if the user were able to change one of those settings that comprise the color_scheme_css setting outside the Customizer, for instance in the Appearance > Background admin page, then the JS-computed color_scheme_css would no longer be valid, right? We can help prevent that if #25569 and #25571 can land.
Thanks! That would be the only problem I can see.
The background screen doesn't matter because it outputs its own <style> tag that will override the theme's styles.
That leaves the header color, which doesn't output its own CSS. Can we just disable the color control on that screen... or add a link to the Customizer? There also seems to be a problem with that color field not showing unless the "Show header text with your image" checkbox is toggled.
#27
@
10 years ago
- Milestone changed from Future Release to 4.1
Twenty Fifteen is hiding the color control on the header screen (which will only be visible to users that can't access the Customizer), so we wouldn't need to worry about that since this would be a theme-specific fix.
Let's get 29988.js.patch polished and punt the core side of this.
Couple notes:
background_colorsetting usespostMessageby default, so we can remove that line.- With #30160, customizer.js will become customize-preview.js, so we'll need to refresh the patch accordingly.
#28
@
10 years ago
Refreshed patch attached.
I removed the line setting background_color to postMessage. Anything else needed to polish this up and get it committed?
#29
@
10 years ago
I meant to include the changes in admin-custom-header.css in a different ticket, but those new rules prevent the color control from showing on the header screen if the header text checkbox is toggled.
This ticket was mentioned in Slack in #core by celloexpressions. View the logs.
10 years ago
#31
follow-up:
↓ 34
@
10 years ago
- Keywords commit added; dev-feedback removed
This looks like a really good solution. I did a thorough review of the patch and it looks good overall. Some of the variables and comments in the color scheme template need work, but let's get the first pass in first, then that should be a quick fix.
This also improves front-end performance since color scheme css is cached in a theme mod, rather than being generated on every page load. In the Customizer, we get actual real-time previewing, as demoed in the screencast above (and you get the instant feedback when tweaking color pickers as well).
This ticket was mentioned in Slack in #core-themes by celloexpressions. View the logs.
10 years ago
#33
@
10 years ago
- Owner set to iandstewart
- Resolution set to fixed
- Status changed from new to closed
In 30274:
#34
in reply to:
↑ 31
@
10 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to celloexpressions:
let's get the first pass in first, then that should be a quick fix.
Reopening because this was a first pass. I blame muscle memory.
@
10 years ago
Customized Twenty Fifteen. The color chooser indicates yellow, but the scheme is custom, not yellow.
This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.
10 years ago
#37
@
10 years ago
If we're worried about the "Customized Yellow" thing, the only reasonable solution at this point would be to change the color scheme control label to "Base Color Scheme", implying that it can/may be customized. Not all elements can be customized, though, such as the purple text color in the purple theme and the dark content background in the dark screen, so I wouldn't go to extreme measures to do anything beyond that change. Additionally, the customized colors may not be visible if there is a header/background image, which is something we should probably address by making the controls contextual or adjusting the descriptive text.
#41
@
10 years ago
- Keywords needs-patch added; has-patch commit removed
- Resolution set to fixed
- Status changed from reopened to closed
Created #30316 for the issue @ryan reported as a separate ticket. Closing this one out.
#42
@
10 years ago
- Keywords has-patch added; needs-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
[30305] introduced some JS that doesn't pass grunt jshint. See 29988.2.patch.
+1.
I started to look at this with @iamtakashi while he was developing the theme. We were thinking about breaking up
$cssintwentyfifteen_color_scheme_css()into selectors for each color to pass to 'js/customizer.js' withwp_localize_scripts()to avoid duplicating all of those selectors.