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

className prefix is not applied when removing "no-js" classname #1031

Closed
keeganstreet opened this Issue Sep 3, 2013 · 12 comments

Comments

Projects
None yet
6 participants
@keeganstreet

keeganstreet commented Sep 3, 2013

The className prefix option is used to prefix the default Modernizr classnames. I am using a prefix of "supports", so I have classes like "supports-js", "supports-no-js", "supports-flexbox" and "supports-no-touch" for my feature detection classnames.

I have added a classname of "supports-no-js" to my HTML element, but Modernizr does not remove this. Modernizr has been hardcoded to remove a classname of "no-js" only, disregarding the className prefix option:

docElement.className = docElement.className.replace(/(^|\s)no-js(\s|$)/, '$1$2') +

This is a feature request to apply the className prefix option when removing the "no-js" className.

@patrickkettner

This comment has been minimized.

Show comment
Hide comment
@patrickkettner

patrickkettner Sep 3, 2013

Member

Just out of curiosity, is there a reason you want to use .supports-no-js, rather than just .no-js?

Member

patrickkettner commented Sep 3, 2013

Just out of curiosity, is there a reason you want to use .supports-no-js, rather than just .no-js?

@keeganstreet

This comment has been minimized.

Show comment
Hide comment
@keeganstreet

keeganstreet Sep 3, 2013

@patrickkettner Yes. I want it to be clear what each classname is for, so all feature-detection classnames are prefixed with supports-.

keeganstreet commented Sep 3, 2013

@patrickkettner Yes. I want it to be clear what each classname is for, so all feature-detection classnames are prefixed with supports-.

@patrickkettner

This comment has been minimized.

Show comment
Hide comment
@patrickkettner

patrickkettner Sep 3, 2013

Member

Sure, its in this case, I think that no-js is clearer. A positive-negative makes it sound clunkier.

Personal preference is all.

Regardless, this is technically an issue with the builder, rather than the detects themselves. I'll be messing with this over at Modernizr/modernizr.com, and will ping you with any update.

Member

patrickkettner commented Sep 3, 2013

Sure, its in this case, I think that no-js is clearer. A positive-negative makes it sound clunkier.

Personal preference is all.

Regardless, this is technically an issue with the builder, rather than the detects themselves. I'll be messing with this over at Modernizr/modernizr.com, and will ping you with any update.

@ryanseddon

This comment has been minimized.

Show comment
Hide comment
@ryanseddon

ryanseddon Sep 3, 2013

Member

We have the option to prefix classes and no-js should be included in that. I'll take a look at this.

Member

ryanseddon commented Sep 3, 2013

We have the option to prefix classes and no-js should be included in that. I'll take a look at this.

@stucox

This comment has been minimized.

Show comment
Hide comment
@stucox

stucox Sep 4, 2013

Member

I think it should remove both prefixed and non-prefixed no-js.

Otherwise, I think it could cause problems if e.g. using HTML5 Boilerplate with className prefix supports- didn't also remove no-js.

@patrickkettner – this is an issue with Modernizr (rather than modernizr.com), as this repo includes the code which applies the no-jsjs switch, and now includes the lib/build.js function which handles build parameters.

Member

stucox commented Sep 4, 2013

I think it should remove both prefixed and non-prefixed no-js.

Otherwise, I think it could cause problems if e.g. using HTML5 Boilerplate with className prefix supports- didn't also remove no-js.

@patrickkettner – this is an issue with Modernizr (rather than modernizr.com), as this repo includes the code which applies the no-jsjs switch, and now includes the lib/build.js function which handles build parameters.

@keeganstreet

This comment has been minimized.

Show comment
Hide comment
@keeganstreet

keeganstreet Sep 4, 2013

Yeah I think that's a valid point @stucox. I wouldn't want this to break existing sites that use the current behaviour, even if the current behaviour isn't completely logical.

keeganstreet commented Sep 4, 2013

Yeah I think that's a valid point @stucox. I wouldn't want this to break existing sites that use the current behaviour, even if the current behaviour isn't completely logical.

@ryanseddon

This comment has been minimized.

Show comment
Hide comment
@ryanseddon

ryanseddon Sep 19, 2013

Member

This will correctly handle class name prefixes now.

@stucox any ideas on testing? We'd probably have to kick off two builds on travis somehow where one does a normal build and the other does a build with the classPrefix config value set.

Member

ryanseddon commented Sep 19, 2013

This will correctly handle class name prefixes now.

@stucox any ideas on testing? We'd probably have to kick off two builds on travis somehow where one does a normal build and the other does a build with the classPrefix config value set.

@stucox

This comment has been minimized.

Show comment
Hide comment
@stucox

stucox Sep 19, 2013

Member

I think you're right. Could be a simpler build though, with just a few tests in it. Maybe our travis task should be running a handful of different configs anyway.

Member

stucox commented Sep 19, 2013

I think you're right. Could be a simpler build though, with just a few tests in it. Maybe our travis task should be running a handful of different configs anyway.

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Feb 17, 2014

If I use the build function from the site this doesn't seem to be solved:
Link

I can't use modernizr-no-js, because it won't be removed from my html. g.className.replace(/(^|\s)no-js(\s|$)/,"$1$2") should be g.className.replace(/(^|\s)modernizr-no-js(\s|$)/,"$1$2").

donaldpipowitch commented Feb 17, 2014

If I use the build function from the site this doesn't seem to be solved:
Link

I can't use modernizr-no-js, because it won't be removed from my html. g.className.replace(/(^|\s)no-js(\s|$)/,"$1$2") should be g.className.replace(/(^|\s)modernizr-no-js(\s|$)/,"$1$2").

@stucox

This comment has been minimized.

Show comment
Hide comment
@stucox

stucox Feb 17, 2014

Member

This change will be in v3.0.0

Member

stucox commented Feb 17, 2014

This change will be in v3.0.0

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Feb 17, 2014

Thank you for the update!

donaldpipowitch commented Feb 17, 2014

Thank you for the update!

patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this issue Feb 22, 2015

@BB-000

This comment has been minimized.

Show comment
Hide comment
@BB-000

BB-000 Sep 1, 2017

When using Modenizr with a class prefix, .no-js was not getting removed. I could fix it by adding the correct prefix to .no-js in my html, however I wanted it to retain its original behaviour, and have either .no-js or .js class on the html (mainly because some third party libraries make use of these classes)

So I replaced
{var r=new RegExp("(^|\s)"+n+"no-js(\s|$)");t=t.replace(r,"$1"+n+"js$2")}
with
{var r=new RegExp("(^|\s)"+"no-js(\s|$)");t=t.replace(r,"$1"+"js$2")}

BB-000 commented Sep 1, 2017

When using Modenizr with a class prefix, .no-js was not getting removed. I could fix it by adding the correct prefix to .no-js in my html, however I wanted it to retain its original behaviour, and have either .no-js or .js class on the html (mainly because some third party libraries make use of these classes)

So I replaced
{var r=new RegExp("(^|\s)"+n+"no-js(\s|$)");t=t.replace(r,"$1"+n+"js$2")}
with
{var r=new RegExp("(^|\s)"+"no-js(\s|$)");t=t.replace(r,"$1"+"js$2")}

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