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

Already on GitHub? Sign in to your account

4095 CSS rule limit in IE #302

Open
stubbornella opened this Issue Sep 6, 2012 · 11 comments

Comments

Projects
None yet
3 participants
Contributor

stubbornella commented Sep 6, 2012

Let's warn people as they approach this limit. I'd suggest a warning at 3800 rules and an error at 4095.

Contributor

nzakas commented Sep 11, 2012

Couple thoughts:

  1. I'm not sure warning at 3800 and at 4095 provides much value over warning just at 4095. Having a 300-rule cushion doesn't seem like something that people should be concerned about.
  2. Remember, each rule can be configured to either warn or error, we give people the ability to specify that manually on the command line. We shouldn't be hardcoding the message level.
Contributor

beckje01 commented Sep 11, 2012

I set them up as two rules so the user could configure them as error or warning as they see fit. I don't think that the message level got hard coded and in order to allow for both levels it needs to be two rules from my understanding.

Contributor

stubbornella commented Sep 12, 2012

For people that run CSS lint on every commit, warning them that they are
approaching a hard limit a bit before they reach it gives them time to
prioritize rule reduction before they see regressions.

As for what the cushion size should be, ideally that could be configurable.

Sent from my iPhone

On Sep 11, 2012, at 10:11 AM, "Nicholas C. Zakas" notifications@github.com
wrote:

Couple thoughts:

  1. I'm not sure warning at 3800 and at 4095 provides much value over
    warning just at 4095. Having a 300-rule cushion doesn't seem like something
    that people should be concerned about.

  2. Remember, each rule can be configured to either warn or error, we
    give people the ability to specify that manually on the command line. We
    shouldn't be hardcoding the message level.


    Reply to this email directly or view it on
    GitHubhttps://github.com/stubbornella/csslint/issues/302#issuecomment-8465159.

Contributor

nzakas commented Sep 12, 2012

@stubbornella Hmmm, I'm not sure I agree with that. Warning someone that they might hit a limit doesn't seem very useful to me. When I'm linting something, I want to know what would be an issue in the code I just wrote rather than a potential issue that might occur sometime in the future (but then again, it might not). There are a whole category of things that might end up being an issue if you continue down that path.

In any event, I think having two rules is overkill. If you think it's a best practice to, for instance, have less than 4000 rules in general, then I would make that a rule with the explanation that there's probably a lot of duplication and there's a potential issue once you reach 4095 in Internet Explorer.

And just an FYI that we don't currently support per rule configuration over and above specifying whether something is a warning or an error.

Contributor

stubbornella commented Sep 12, 2012

@nzakas It seems we disagree, so I thought the best thing to do was to ask the community what would be most useful to them. https://twitter.com/stubbornella/status/245952112243138561

Contributor

stubbornella commented Sep 12, 2012

ok, I let people chime in for around a half an hour. They prefer both an error at 4095 and warning a bit in advance of that. In addition, if we have both, anyone who doesn't like it can turn off the warning rule. Win-win.

7- error only,
3 - warning only,
15 - both

So, let's go ahead with two rules, one for the error and one for the warning.

Contributor

stubbornella commented Sep 12, 2012

One interesting comment came up. There may be different limits for IE7 versus 8 related to media queries. I'm waiting to hear back from @jaffathecake about that.

https://twitter.com/jaffathecake/status/245955436396826625

Contributor

nzakas commented Sep 12, 2012

Well, my opinion hasn't changed, but go for it. Just make sure the two different rules are in two different files with two different sets of tests (the current pull request has both rules in the same file).

Contributor

beckje01 commented Sep 17, 2012

Pull request #304 has been updated

Contributor

stubbornella commented Sep 18, 2012

Looks good to me, I'd just change the browsers affected to only include IE,
rather than "all".

Nicole

Sent from my iPhone

On Sep 18, 2012, at 1:07 AM, Jeff Beck notifications@github.com wrote:

Pull request #304 stubbornella#304 has
been updated


Reply to this email directly or view it on
GitHubhttps://github.com/stubbornella/csslint/issues/302#issuecomment-8634247.

Contributor

beckje01 commented Sep 21, 2012

@stubbornella Both are not changed to only IE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment