Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeytown2’s picture

Looks like media queries do not change the css selector count.

mikeytown2’s picture

Status: Active » Fixed
FileSize
4.59 KB

The following patch has been committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mikeytown2’s picture

Title: Integrate this module » Limit CSS selector count to 4095
branana’s picture

Status: Closed (fixed) » Needs review
FileSize
7.84 KB

I have create a patch that allows advagg to part out a single source CSS file over 4095 rules long into multiple parts first, then include them individually as style tags after aggregation.

This patch does assume that there are no media query blocks spanning the rules where it needs to split up the files at. (AKA: around every 4000th rule or about there).

mikeytown2’s picture

Status: Needs review » Needs work

This patch does assume that there are no media query blocks spanning the rules where it needs to split up the files at. (AKA: around every 4000th rule or about there).

I have a function advagg_parse_media_blocks() that I use for breaking up a CSS file into media query sections. Example usage can be found in this bug that I just fixed #2124391: Off by one error with advagg_parse_media_blocks (was actually working on an example on how to use it for here when I discovered the bug). If you could incorporate the usage of advagg_parse_media_blocks() into your patch so that it will correctly handle large media query blocks that would be helpful :)

mikeytown2’s picture

Issue summary: View changes

Updated issue summary.

branana’s picture

OK I will see if I can implement advagg_parse_media_blocks() in the split line finding part

mikeytown2’s picture

@branana
Any progress?
:)

mikeytown2’s picture

Assigned: Unassigned » mikeytown2
Issue summary: View changes

Going to work on this

mikeytown2’s picture

Status: Needs work » Fixed
FileSize
12.27 KB

This patch has been committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

eduardo.flores’s picture

Version: 7.x-2.x-dev » 6.x-1.x-dev
Status: Closed (fixed) » Active
Issue tags: +needs backport to 6.x-1.x
eduardo.flores’s picture

Status: Active » Needs review
FileSize
7.46 KB

I worked on this patch for Drupal 6 where it will divide the css file groups in subgroups if it exceeds the selector limit.

malcomio’s picture

The patch in #13 seems to work as expected from an initial test - will test some more before marking as RTBC though. Also there are a couple of typos and coding standards issues in the patch - will add a new patch later.

malcomio’s picture

After applying the patch in #13 and enabling advagg_bundler, the advagg_bundler_selector_count table isn't actually created, because there isn't a hook_install calling drupal_install_schema()

Here's a new patch

joelpittet’s picture

Status: Needs review » Needs work

@malcomio could you post an interdiff so we can see what you changed? @see https://drupal.org/documentation/git/interdiff

+++ b/advagg_bundler/advagg_bundler.admin.inc
@@ -23,10 +23,10 @@ function advagg_bundler_admin_settings_form() {
-    '#type' => 'checkbox',
-    '#title' => t('Bundler is Active'),
-    '#default_value' => variable_get('advagg_bundler_active', ADVAGG_BUNDLER_ACTIVE),
-    '#description' => t('If not checked, the bundler will passively monitor your site, but it will not split up aggregates.'),
+    '#type'           => 'checkbox',
+    '#title'          => t('Bundler is Active'),
+    '#default_value'  => variable_get('advagg_bundler_active', ADVAGG_BUNDLER_ACTIVE),
+ '#description'    => t('If not checked, the bundler will passively monitor your site, but it will not split up aggregates.'),

This type of indentation is not in our coding standards. https://drupal.org/coding-standards

Also changing the indentation makes it hard to see what part of the code is actually fixing the bug/feature in the issue so please avoid going all out and move the cleanup coding standards stuff to it's own issue(usually).

malcomio’s picture

Status: Needs work » Needs review
FileSize
5.66 KB

Good point - here's a new patch without the coding standards changes, and an interdiff.

malcomio’s picture

mikeytown2’s picture

Assigned: mikeytown2 » Unassigned

Just a heads up that the 7.x version of this has a couple of bugs that I will be working on
#2405531: IE CSS Selectors limiter cause memory and I/O issues
#2412093: IE CSS Selectors limiter doesn't work with less module

  • mikeytown2 committed 677a1e3 on 6.x-1.x authored by malcomio
    Issue #1983728 by malcomio, mikeytown2, branana, eduardo.flores: Limit...
mikeytown2’s picture

Status: Needs review » Fixed

Thanks for the patch. Looking over it and the 7.x issue had to do with stream wrappers.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.