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

classPrefix is not working #20

Open
Tokimon opened this issue Oct 13, 2015 · 14 comments
Open

classPrefix is not working #20

Tokimon opened this issue Oct 13, 2015 · 14 comments

Comments

@Tokimon
Copy link

Tokimon commented Oct 13, 2015

I have tried to create a build with the setting classPrefix: 'has-' from gulp, but the setting doesn't work, and as the gulp-modernizr is really just passing in the config to customizr, I write the issue here:

  1. The crawler doesn't find any properties has-xxx, so correct tests are not initialized.
  2. The prefix is never passed on to the Modernizr script.

I did some digging and it seems that the legacy property cssprefix is still used in the code:

  // customizr/src/crawler.js line 57
  // (method findStringMatches)

  prefix = settings.cssprefix || '';

Hence I need to pass in cssprefix: 'has-' instead of classPrefix: 'has-' before it works, but that contradicts the fact that the property name has been changed to classPrefix (see #8).

If I change the line indicated to prefix = settings.classPrefix || '' the crawler finds the correct properties with the has- prefix.

But unfortunately the prefix is still not passed on to the final Modernizr script and right now I have no precise idea why.

Below is the Gulp setting I have used:

  return gulp.src([path.resolve('.tmp/index.css'), path.resolve('.tmp/index.js')])
    .pipe(modernizr({
      "classPrefix": "has-",
      "options": [ "html5printshiv", "html5shiv", "setClasses" ],
    }))
    .pipe(uglify())
    .pipe(gulp.dest(config.destination));

As a side note; I think it is a bit strange that setClasses is not added to the default list of options, as personally I have never used Modernizr without wanting to use the classes as well.

Related issues
Modernizr/grunt-modernizr#30
#8

@trikadin
Copy link

+1

@mahnunchik
Copy link

Any news?

@gmhenderson
Copy link

+1

@etiennetalbot
Copy link

+1...

@dasblitz
Copy link

It seems this functionality is already merged into develop, not yet in master though :(

in customizr/src/builder.js on line 47 replace the following:

var modernizrOptions = {
    "feature-detects": tests,
    "options": options,
    "minify": minify,
    "dest": settings.dest
};

with:

var modernizrOptions = {
    "feature-detects": tests,
    "options": options,
    "minify": minify,
    "dest": settings.dest,
    "classPrefix": settings.classPrefix
};

now you can use it in your gulpfile like so:

.pipe(modernizr('modernizr-custom.js', {
    options : ['setClasses'],
    classPrefix: 'supports-'
})) 

@Tokimon
Copy link
Author

Tokimon commented Apr 14, 2016

Perfect. I will try it out 👍

@mm-tom
Copy link

mm-tom commented Aug 5, 2016

+1

@tdmalone
Copy link

thanks @dasblitz, you'll also need to update crawler.js at line 57 if you want to pick up the prefixed properties on crawl. Better yet, apply each of the changes that this commit makes (props to @michaelisjones in the above referenced issue)

@yoannisj
Copy link

yoannisj commented Sep 29, 2016

+1! Apparently, the fix was already merged in the develop branch pull request #16 . Please also apply to master branch, so dependent packages/modules can also benefit this feature.

@toh82
Copy link

toh82 commented Mar 31, 2017

Any update on this, seems it's still not included in the version with comes with npm install, could we make that happen, soon?

@lvl99
Copy link

lvl99 commented Sep 28, 2017

For the love of Jebus, someone, anybody, please merge this into master...

@gregmatys
Copy link

+1

@rejas
Copy link
Member

rejas commented Jul 3, 2018

finally managed to get the fix merged and released in v1 @Tokimon can you check and close this issue?

@Tokimon
Copy link
Author

Tokimon commented Oct 2, 2018

Apparently I don't get my messages from github, so I only just saw this.
I no longer have the build set up with this, but I will reconstruct it and verify once I have some time.
But great job!

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