Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Trac 35395 (Custom CSS) #154

Closed
wants to merge 71 commits into from

Conversation

@johnregan3
Copy link

commented Sep 8, 2016

Opening PR for Feedback. DO NOT MERGE.

35395

At this point, everything works (although there is a bug where changes display, then disappear in the Preview). There is still plenty to do, but the implementation is functional.

Seeking feedback on the following questions in particular:

  • Class WP_Custom_CSS
  • I used a Class for the following reasons:
    - It uses several methods specific to this feature (e.g., the transient)
    - It can be extended by theme/plugin authors.
    - We can use class constants to make modifying the CPT slug and transient name simpler (DRY).
  • Where should this class be fired? Currently, it's at the bottom of the file.
  • Is it appropriate to use the CPT slug and transient name as constants/properties?
  • Is the slug name "style" appropriate for this CPT? I kept it separate from "custom-css" because it is quite possible that plugins are using this term.
  • I'd like to do CSSTidy and Custom CSS-specific Notifications within this class. Your thoughts?
  • JavaScript
  • Is the new code in customize-controls.js implemented correctly? (note that I still have a bug in the Preview to fix there)
  • File placement
    • CodeMirror is found in wp-includes/js/codemirror/. Is this appropriate?
    • Where should CSSTidy be located?

Done

In this branch, I've done the following so far (with some foundational work from Nick Halsey):

  • Created the section for Custom CSS in the customizer
  • Created a new Controller, "Code Editor"
  • Added foundational JS for the Code Editor control
  • Installed CodeMirror
  • Created a CPT called "style" to hold our custom CSS
    • Note: You can currently view the content of the posts at wp-admin/edit.php?post_type=style
  • Created a custom Setting called "WP_Customize_CSS_Setting"
  • Configured Setting to save to style CPT

To Do

  • Fix bug when updating the Preview (changes flash correctly for a moment, then disappear)
  • Implement CSSTidy
  • Implement Notifications
  • Add hooks
  • Escaping/Validation/Sanitization throughout
  • Make CPT hidden
  • Test on Networks
  • Unit Tests

@westonruter @MattGeri @valendesigns @kienstra @delawski

@johnregan3 johnregan3 assigned westonruter and MattGeri and unassigned MattGeri Sep 8, 2016
'add_new_item' => __( 'Add New Style' ),
),
'public' => true,
'show_ui' => true,

This comment has been minimized.

Copy link
@celloexpressions

celloexpressions Sep 8, 2016

Collaborator

This should probably be false. We'll add UI for revisions later, likely as part of snapshots/transactions and other customizer revisions.

This comment has been minimized.

Copy link
@johnregan3

johnregan3 Sep 8, 2016

Author

@celloexpressions Most definitely. I just have it this way currently so I can view the post content when the post is saved. 😄

// @todo only "title" and "revisions" after testing.
'supports' => array( 'title', 'editor', 'author', 'revisions' ),
'capabilities' => array(
'delete_posts' => 'edit_theme_options',

This comment has been minimized.

Copy link
@celloexpressions

celloexpressions Sep 8, 2016

Collaborator

We might consider using edit_files for non-multisite. Something to discuss in the core dev chat probably.

Underlining links in the customizer was a recent change in core.
$this->add_control( 'wp_custom_css_more', array(
'type' => 'none',
'section' => 'custom_css',
'description' => sprintf( '%s<br /><a href="%s" class="external-link" target="_blank">%s&nbsp;<span class="screen-reader-text">(%s)</span></a>.',

This comment has been minimized.

Copy link
@celloexpressions

celloexpressions Sep 8, 2016

Collaborator

I think we should probably make the parenthesis translatable as well.

*/
/**
* A setting that is used to filter a value, but will not save the results.

This comment has been minimized.

Copy link
@celloexpressions

celloexpressions Sep 8, 2016

Collaborator

Need to update this description, although I'm also wondering whether this custom setting could be useful in plugins if it were a bit more generic. Something to think about.

@celloexpressions

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2016

This is a great start! I think style could run into conflicts as a post type slug as well. I wouldn't be opposed to doing wp_custom_css if we're concerned about plugin compatibility with custom_css. Plugins will need to update to provide a migration path to the core feature (and in cases where they want to, extend the core feature), so we may be able to get away with custom_css and some sort of a check to avoid overwriting existing styles if they follow the same post-name conventions.

I made a couple little tweaks and added some comments inline as well. The changes to customize-controls.js generally look correct, although the section-expansion binding could probably be improved. That should be the right place for CodeMirror, and CSSTidy would probably be in wp-includes/CSSTidy, unless we can get it down to one file, which could probably go in wp-admin/includes/, although I think that's less common and this seems like more of a wp-includes thing.

@celloexpressions

This comment has been minimized.

Copy link
Collaborator

commented on src/wp-includes/theme.php in 807fc30 Sep 12, 2016

This needs an || is_customize_preview() for selective refresh to work.

This comment has been minimized.

Copy link
Author

replied Sep 12, 2016

This is one of the places where I was running into issues. When I added the bidirectional data binding in the JS, the preview would refresh twice, and the second time, it would add a something like this:

     <style id="wp-custom-css">
          <style id="wp-custom-css">
               ...
          </style>
     </style>

Please correct me if I'm wrong, but I believe because I have the data bound in the JS, it updates automatically without needing selective refresh.

@celloexpressions

This comment has been minimized.

Copy link
Collaborator

replied Sep 12, 2016

The JS preview will show the instant results, but selective refresh is also needed to provide the full preview with sanitization, etc. applied from PHP. If there are differences between the two they'll show up on a slight delay when the selective refresh kicks in.

I'm not sure how the bidirectional part affects this though, @westonruter?

This comment has been minimized.

Copy link
Collaborator

replied Sep 12, 2016

Make sure that the render_callback only prints the content of the style tags, not the style tags themselves. The contents of the partial selector are replaced but the container stays in a selective refresh.

This comment has been minimized.

Copy link
Collaborator

replied Sep 12, 2016

A partial can be registered with container_inclusive as true or false. If false, then the partial is expected to only render the contents of the CSS.

And yes, selective refresh can be skipped here. No need to register a partial because you can use pure postMessage to inject the CSS as-is into the style element.

This comment has been minimized.

Copy link
Collaborator

replied Sep 12, 2016

So:

This needs an || is_customize_preview() for selective refresh to work.

Is for selective refresh or updating just via JS directly. There needs to be an element output that can be targeted in either case.

@johnregan3

This comment has been minimized.

Copy link
Author

commented Oct 14, 2016

@celloexpressions @westonruter

This is ready for another review pass, so that if you have some time this weekend we can get a head start on next week. I've updated this branch quite a bit, and and I think it cleaned up the feature very nicely.

  • Removed Code Editor control
  • Simplified the Section help icon usage greatly
  • Renamed wp_custom_css to custom_css throughout per @celloexpressions 's request
  • Use the theme name in the Setting ID
  • All CSS now resides inside customize-controls.css
  • Implemented has_filter() in the Setting per @westonruter
  • For get_value(), we now check for a customized state
  • Moved the CPT into create_initial_post_types()
  • Completely removed that crazy post creation bit. Now it's only created in update_setting()
  • Several other bits and bobs mentioned in your reviews

I still have these things to do:

  • Save the Post ID into a theme_mod
  • Add a filter to the CSS output
  • Work out removing those string literals when the CSS is validated. (this could be tricky)
  • Maybe move the icon description toggle into its own function so it can be used by both Panels and Sections
  • Update/Write tests

If this is not a complete list, please let me know so I don't miss anything.

*
* @param string $css CSS pulled in from the Custom CSS CPT.
*/
return apply_filters( 'custom_css_output', $css );

This comment has been minimized.

Copy link
@johnregan3

johnregan3 Oct 17, 2016

Author

@celloexpressions @westonruter Does this filter name seem appropriate?

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 17, 2016

Collaborator

Perhaps wp_get_custom_css instead. Compare with the get_custom_logo filter.

Copy link
Collaborator

left a comment

This is looking good; I just found a couple more minor tweaks to adjust then we should be good to go.

@@ -53,6 +53,10 @@ body {
margin-bottom: 15px;
}

#customize-controls .customize-info.section-meta {
margin-bottom: 10px;

This comment has been minimized.

Copy link
@celloexpressions

celloexpressions Oct 17, 2016

Collaborator

Typical spacing between section headings is 15px.

#customize-controls .customize-info .customize-panel-description.open + .no-widget-areas-rendered-notice {
border-top: none;
}

#customize-controls .customize-info .customize-panel-description p:first-child {
#customize-controls .customize-info .customize-section-description {
margin-bottom: 10px;

This comment has been minimized.

Copy link
@celloexpressions

celloexpressions Oct 17, 2016

Collaborator

Should be 15px

}

content = meta.find( '.customize-section-description:first' );
if ( meta.hasClass( 'open' ) ) {

This comment has been minimized.

Copy link
@celloexpressions

celloexpressions Oct 17, 2016

Collaborator

These could be condensed by using toggleClass, slideToggle, and:

.attr('aria-expanded', function ( i, attr ) {
    return attr === 'true' ? 'false' : 'true';
});
johnregan3 added 4 commits Oct 17, 2016
@johnregan3

This comment has been minimized.

Copy link
Author

commented Oct 18, 2016

In my latest work, I've done the following:

  • PHPUnit Tests for WP_Customize_Custom_CSS_Setting
  • Changed Margin around Section title as requested by @celloexpressions
  • Updated Help Icon open/close JS, per @celloexpressions
  • Implemented a secondary check for content: if a validation error in balanced characters is found. (this is in lieu of removing those string literals for the time being)

I've opted to not move the help icon toggling into a new function as I don't really have time to implement/test it.

westonruter added 2 commits Oct 18, 2016
…ss; ensure customized value is previewed; use WP_Customize_Setting methods instead of filters
if ( ! $post_id ) {

// Cache value in the theme mod to eliminate a query with each request.
$post = get_page_by_title( $stylesheet, OBJECT, 'custom_css' ); // @todo This needs to look up by post_name instead since it is indexed.

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 19, 2016

Collaborator

We should look-up by post_name instead of post_title since the former is indexed. Changesets provides an example of this.

}
} else {
// Theme is not active, so the cached post ID cannot be sourced from the theme mods.
$post = get_page_by_title( $stylesheet, OBJECT, 'custom_css' ); // @todo This needs to look up by post_name instead since it is indexed.

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 19, 2016

Collaborator

Ditto. We should look-up by post_name instead of post_title since the former is indexed. Changesets provides an example of this.

* @return string Limited sanitized CSS.
*/
public function sanitize( $css ) {
return parent::sanitize( wp_sanitize_css( $css ) );

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 19, 2016

Collaborator

Does wp_sanitize_css() provide any value now that we have unfiltered_css? What does it sanitize for us? If it helps, then we need unit tests to show what it does.

* for non-active themes.
*/
if ( $this->manager->get_stylesheet() !== $this->stylesheet ) {
return false;

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 19, 2016

Collaborator

Actually we can remove this conditional. We can just only do set_theme_mod() if the stylesheet matches.

'delete_with_user' => false,
'can_export' => true,
'_builtin' => true, /* internal use only. don't use this when registering your own post type. */
'supports' => array( 'title', 'revisions' ),

This comment has been minimized.

Copy link
@westonruter

westonruter Oct 19, 2016

Collaborator

I think that we can remove revisions support until we have a UI to show the revisions?

@westonruter

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.