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.

Sign up for GitHub

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

Jump to bottom

Remove "no-" prefix for all properties? #808

Closed
alexslade opened this issue Feb 11, 2013 · 8 comments · Fixed by #844
Closed

Remove "no-" prefix for all properties? #808

alexslade opened this issue Feb 11, 2013 · 8 comments · Fixed by #844
Assignees
@stucox
Milestone
Modernizr v3.0

Comments

@alexslade
Copy link

Adding a manual class of "no-js" is useful for styling, and gets removed when js is enabled. It would be useful if this also happened for other properties.

For instance, if I set 'no-backgroundsize' on my page, I can style a fallback for people with older browsers OR javascript-disabled.
But, this doesn't seem to be removed once Modernizr detects that feature.
@stucox
Copy link
Member

stucox commented Feb 11, 2013

Good call. Reckon you could help us out by putting together a pull request? This is probably where that should go: https://github.com/Modernizr/Modernizr/blob/master/src/setClasses.js

@alexslade
Copy link
Author

@stucox Yea, no problem, though it'll be a couple of days before I can sort it.

@alexslade alexslade mentioned this issue Feb 28, 2013
Merged
@alexslade
Copy link
Author

@stucox Here's my first crack, mind giving me some feedback?

@stucox stucox mentioned this issue Mar 23, 2013
Merged
@stucox
Copy link
Member

stucox commented Mar 23, 2013

Fixed by #874

@stucox stucox closed this as completed Mar 23, 2013
@stucox stucox mentioned this issue Mar 23, 2013
Closed
@stucox stucox mentioned this issue Apr 8, 2013
Open
@stucox
Copy link
Member

stucox commented Apr 22, 2013

@heeton - I'm afraid I'm tempted to revert this change. Reopening for discussion.

I think putting no-<feature> classes on the <html> element is a bad idea (except no-js), for several reasons:

1. It's likely to cause confusion.

Example:

.module {
  background: url(background1x.jpg);
  /* This won't be applied if `.no-backgroundsize` matches, right? Wrong. */
  background-size: 200%;
}
.no-backgroundsize .module {
  background: url(background2x.jpg);
}

This looks fine at first glance, but if JS isn't supported and I've got a no-backgroundsize class on my <html> element, I'll get a 4x-sized background.

2. Async tests are complicated enough when using Modernizr's styling classes.

Here's a simple pattern for WebP (which is an async test) with a fallback:

.no-webp .container {
  background-image: url('image.jpg');
}
.webp .container {
  background-image: url('image.webp');
}

The .no-webp class is required on the first rule, otherwise it'll match while the result is pending and cause 2 requests if WebP then happens to be supported. If you manually add a no-webp class to the <html> element, I don't think there's any way (from CSS alone) to avoid causing 2 requests when WebP is supported while still providing a fallback. Obviously this is a big deal, because the whole point of WebP is to save bandwidth!

3. You can use .no-js

I don't know of any case which couldn't be solved like this:

.no-js .module,
.no-backgroundsize .module {
  /* shared styles */
}

Admittedly some cases might be much more verbose... but that's what CSS preprocessors are there to solve.

All these caveats would be hard to document clearly... which usually means it's a bad idea.

@stucox stucox reopened this Apr 22, 2013
@ghost ghost assigned stucox Aug 19, 2013
@stucox
Copy link
Member

stucox commented Nov 27, 2013

Silence for 7 months = I’m reverting this. PR incoming.

@alexslade
Copy link
Author

@stucox Sorry, totally missed this :/ Yea, go ahead, I agree with your points.

@stucox stucox mentioned this issue Nov 27, 2013
Merged
@stucox
Copy link
Member

stucox commented Nov 27, 2013

Resolved by #1122

@stucox stucox closed this as completed Nov 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stucox stucox

Labels
None yet
Projects
None yet
Milestone
Modernizr v3.0
Development

Successfully merging a pull request may close this issue.

Remove "no-<feature>" classes if feature is detected alexslade/Modernizr
2 participants
@alexslade @stucox