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

Option to specify 'classPrefix' is missing #30

Closed
daveschoutens opened this issue Oct 28, 2013 · 6 comments
Closed

Option to specify 'classPrefix' is missing #30

daveschoutens opened this issue Oct 28, 2013 · 6 comments

Comments

@daveschoutens
Copy link

Users are unable to specify a class prefix for Modernizr to use.

As an aside: adding this feature should also entail enhancing the CSS parsing logic to account for such a prefix.

Example: If the classPrefix is set to 'mod-', then the CSS parser should look for class names such as .mod-placeholder instead of simply .placeholder.

@doctyper
Copy link
Member

Thanks! This bug is fixed as of 0.3.1-1. You can define a prefix using the extensibility.prefixed flag:

"extensibility": {
    ...
    "prefixed": "foo"
}

@daveschoutens
Copy link
Author

Wow! That was fast :-). I was going to dig into the code and contribute a fix myself after work, but you beat me to it. Thanks!

@daveschoutens
Copy link
Author

Hmmm, on second look, this isn't quite what I expected it to be. I assumed that if an optional prefix was specified, that non-prefixed versions of the class would NOT be considered hits.

For example, if Modernizr is configured with a prefix of mod and thus producing classes such as .mod-placeholder, then I would expect a class named .placeholder not to be a match, because that class can no longer be considered a Modernizr class.

False positives like this will cause grunt-modernizr to produce a version of Modernizr containing unused tests, thus bloating the file needlessly.

Now that we are getting false-positives for unprefixed class names, I'll go ahead and add a plug for issue #29 here as well, which describes a related issue whose fix will involve changing the same code.

@doctyper
Copy link
Member

Ah, you are correct. That'll teach me to rush a release. I'll look into it.

@doctyper doctyper reopened this Oct 28, 2013
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…sed to search within JS vs CSS files separate; eliminate possibility of getting a false-positive match if the test name is merely the prefix / substring of another class; Added proper support matching on prefixed classnames in CSS files
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…sed to search through JS vs CSS files separate; eliminate possibility of getting a false-positive match if the test name is merely the prefix / substring of another class; Added proper support matching on prefixed classnames in CSS files.
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…sed to search through JS vs CSS files separate; eliminate possibility of getting a false-positive match if the test name is merely the prefix / substring of another class; Added proper support matching on prefixed classnames in CSS files.
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…Exp for JS files vs CSS(-ish) files. Takes optional class prefix into account
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…Exp for JS files vs CSS(-ish) files. Takes optional class prefix into account
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…Exp for JS files vs CSS(-ish) files. Takes optional class prefix into account
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…ranch). Involved using separate RegExp for JS files vs CSS(-ish) files. Takes optional class prefix into account
daveschoutens added a commit to daveschoutens/grunt-modernizr that referenced this issue Nov 6, 2013
…ranch). Involved using separate RegExp for JS files vs CSS(-ish) files. Takes optional class prefix into account
@doctyper
Copy link
Member

@daveschoutens I see you've made progress on this. Need me to take a look, or is it ready to rock?

@daveschoutens
Copy link
Author

Yes, I've made progress. I was going to submit a pull request, but the code I wrote is for the feature/modernizr-3.0 branch, and the interface didn't seem intelligent enough to tell my commit wasn't intended for master, so I decided to put it off and do some more testing first. Some time has passed and I am confident in the changes now that I've been actively using the fix and not run into any issues.

As my commit comment suggested, my code fixes the 'false positives' problem as well as provides a 'classPrefix' option. Note that I opted not to piggy-back this logic on the 'extensibility.prefixed' option, since in the online Modernizr download tool, the CSS prefix and the extensibility options are separate as well. Looking at the code, it appears that they refer to two completely different things.

In short, I think it's ready to rock! I'll go ahead and submit the PR (even though it can't be auto-merged). Please let me know if you need me to do anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants