+ Adds a new Tabbed Settings page #27
Conversation
shadyvb
commented
Aug 25, 2013
shadyvb
commented
- Adds option to disable minification alltogether
- Adds option to disable minification based on specific conditions ( islogged, is_admin, query var present )
- Adds option to exclude resources, line-delimited, based on string search not matching, means it matches any part of the url
- Note: Disabling minification does not disable serving minified files, so cached pages does not break.
+ Adds option to disable minification alltogether + Adds option to disable minification based on specific conditions ( islogged, is_admin, query var present ) + Adds option to exclude resources, line-delimited, based on string search not matching, means it matches any part of the url * Note: Disabling minification does not disable serving minified files, so cached pages does not break.
…es. xwp#5 * Fix a small translation missing text domain
* Remove trailing spaces
|
Trying this functionality out, because s2member plugin doing some funky stuff with its enqueueing and needs to be excluded. First comment: commit messages should be better. http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html |
|
Second, I think you have some whitespace issues in a7f751e visible in at least the constants section in the very start of the commit. You should not be changing constant order or upstream code whitespace format. |
|
Re: commit messages, yes they should ideally be composed according to that note. Although the prefixing of each line with
or
Not a huge deal though. |
|
Functionality: what I'd like to see for URL exclusion is also a filter implementation, so code-based overrides become possible. Then admin UI should also use the filter to do its thing. Starting with just the filter would probably be the minimal viable product that has a chance of getting merged quick. |
|
Meh, I guess the filter is already done https://github.com/x-team/wp-dependency-minification/blob/master/dependency-minification.php#L707 |
|
Yes, re: filter, see the forums: http://wordpress.org/support/topic/woocommerce-not-supported?replies=2#post-4555036 |
|
| @@ -49,7 +54,21 @@ static function setup() { | |||
| || | |||
| in_array( $GLOBALS['pagenow'], array( 'wp-login.php', 'wp-register.php' ) ) | |||
| ); | |||
| if ( $is_frontend ) { | |||
| $disabled = ( | |||
| ! empty( self::$options['disabled_on_conditions']['all'] ) | |||
There was a problem hiding this comment.
@shadyvb Notice: Undefined index: all
| <?php esc_html_e( 'Administrators', 'depmin' ) ?> | ||
| </label> | ||
| <label for="options[disabled_on_conditions][queryvar][enabled]"> | ||
| <input type="checkbox" name="options[disabled_on_conditions][queryvar][enabled]" id="options[disabled_on_conditions][queryvar][enabled]" <?php echo checked( self::$options['disabled_on_conditions']['queryvar']['enabled'] ) ?> value="1"> |
There was a problem hiding this comment.
You don't have to call "echo" for checked() function.
|
Did some fixes to comply with WPCS in code, not only related to PR, only remaining issues are mostly related to indentation. UPDATE: @westonruter I just figured out that i shouldn't have fixed CS issues on this PR, it'd make it quite hard for code review. Sorry! |
| @@ -186,7 +204,8 @@ static function admin_notices() { | |||
| ?> | |||
| <div class="error"> | |||
| <p><?php | |||
| echo sprintf( | |||
| echo esc_html( | |||
There was a problem hiding this comment.
I didn't test this yet.. but how you use esc_html with some HTML tags in the string? Can you check this.
There was a problem hiding this comment.
Good catch! Fixed in shadyvb@a18558f
Adds a new Tabbed Settings page
|
This has been open too long, sorry @shadyvb. Now that it is in |
|
Opened PR #44 to track additional fixes for PHP_CodeSniffer, and for the 1.0 release. |
