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

Feature Request: Option to disable HTML Compression for Logged-In Users #208

Closed
wants to merge 12 commits into from

Conversation

renzms
Copy link
Contributor

@renzms renzms commented Jan 22, 2016

@@ -345,6 +346,7 @@ public function setup()
'htmlc_compress_css_code' => '1', // `0|1`.
'htmlc_compress_js_code' => '1', // `0|1`.
'htmlc_compress_html_code' => '1', // `0|1`.
'htmlc_when_logged_in' => '0', // `0|1`; enable when logged in?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renzms I noticed the formatting is slightly off here; notice how your addition here doesn't line up with the options above it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renzms I noticed the formatting is still off on this line.

@raamdev
Copy link
Contributor

raamdev commented Jan 25, 2016

@renzms It looks like all that's needed now is to check this option before running the HTML Compressor when users are logged in. Can you find where that would go?

@renzms
Copy link
Contributor Author

renzms commented Jan 26, 2016

@raamdev

I noticed the formatting is slightly off here; notice how your addition here doesn't line up with the options above it.

Thanks, I didn't catch the alignment in Atom.

It looks like all that's needed now is to check this option before running the HTML Compressor when users are logged in. Can you find where that would go?

Where would that go?

@raamdev
Copy link
Contributor

raamdev commented Jan 27, 2016

Where would that go?

Were you able to look? Any ideas where it might go?

@renzms
Copy link
Contributor Author

renzms commented Jan 28, 2016

@raamdev

Were you able to look? Any ideas where it might go?

After this: line here?

@@ -840,6 +840,12 @@ public function __construct()
echo ' <p><input type="text" name="'.esc_attr(GLOBAL_NS).'[saveOptions][htmlc_cache_expiration_time]" value="'.esc_attr($this->plugin->options['htmlc_cache_expiration_time']).'" /></p>'."\n";
echo ' <p class="info" style="display:block;">'.__('<strong>Tip:</strong> the value that you specify here MUST be compatible with PHP\'s <a href="http://php.net/manual/en/function.strtotime.php" target="_blank" style="text-decoration:none;"><code>strtotime()</code></a> function. Examples: <code>2 hours</code>, <code>7 days</code>, <code>6 months</code>, <code>1 year</code>.', SLUG_TD).'</p>'."\n";
echo ' <p>'.sprintf(__('<strong>Note:</strong> This does NOT impact the overall cache expiration time that you configure with %1$s. It only impacts the sub-routines provided by the HTML Compressor. In fact, this expiration time is mostly irrelevant. The HTML Compressor uses an internal checksum, and it also checks <code>filemtime()</code> before using an existing cache file. The HTML Compressor class also handles the automatic cleanup of your cache directories to keep it from growing too large over time. Therefore, unless you have VERY little disk space there is no reason to set this to a lower value (even if your site changes dynamically quite often). If anything, you might like to increase this value which could help to further reduce server load. You can <a href="https://github.com/websharks/HTML-Compressor" target="_blank">learn more here</a>. We recommend setting this value to at least double that of your overall %1$s expiration time.', SLUG_TD), esc_html(NAME)).'</p>'."\n";
echo ' <h3>'.__('Do NOT use Html Compression for logged in users?', SLUG_TD).'</h3>'."\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renzms Please change this to "Do NOT use HTML Compression for logged-in users?" (Note the changes to: 'HTML' and 'logged-in'.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renzms Actually, please change it to the following (I just realized we're going to be defaulting this option to disabled, so the title should reflect that); change it to the following please:

Enable HTML Compression for logged-in users?

@raamdev
Copy link
Contributor

raamdev commented Feb 24, 2016

After this: line here?

Right, except I would put a block just like this one right underneath it for HTMLC_WHEN_LOGGED_IN and then there should be a conditional just like this one right underneath it that checks if ($self->is_user_logged_in && !COMET_CACHE_HTMLC_WHEN_LOGGED_IN).

@renzms
Copy link
Contributor Author

renzms commented Feb 25, 2016

Right, except I would put a block just like this one right underneath it for HTMLC_WHEN_LOGGED_IN and then there should be a conditional just like this one right underneath it that checks if ($self->is_user_logged_in && !COMET_CACHE_HTMLC_WHEN_LOGGED_IN).

@raamdev Added changes, ready for review! Thanks.

@raamdev
Copy link
Contributor

raamdev commented Feb 25, 2016

@renzms I added another note here: #208 (comment)

@renzms
Copy link
Contributor Author

renzms commented Feb 26, 2016

@raamdev Spacing fixed, ready for review! (again 👍 ) - 142c3ce

@raamdev
Copy link
Contributor

raamdev commented Feb 26, 2016

@renzms I improved the options panel a bit; see acae086.

Here's a screenshot of the new section in HTML Compression as of now:

Edit: See below.

@raamdev
Copy link
Contributor

raamdev commented Feb 26, 2016

@jaswsinc This is ready for a 2nd reviewer when you get a chance. :-) I've tested this feature branch to confirm it works as expected.

@raamdev
Copy link
Contributor

raamdev commented Feb 26, 2016

Updating screenshot; fixed a few things in the description.

Edit: See below.

@raamdev
Copy link
Contributor

raamdev commented Feb 26, 2016

@jaswsinc Final screenshot, sorry:

2016-02-26_09-02-43

@jaswrks
Copy link
Contributor

jaswrks commented Feb 27, 2016

@raamdev I reviewed your UI enhancements with the improved description of this option. Looks great to me. Nice!

@raamdev raamdev closed this Feb 27, 2016
@raamdev raamdev deleted the feature/650 branch February 27, 2016 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants