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

csscalc always return false if usePrefixes is false #1852

Closed
zenatti opened this issue Jan 24, 2016 · 9 comments · Fixed by #1854
Closed

csscalc always return false if usePrefixes is false #1852

zenatti opened this issue Jan 24, 2016 · 9 comments · Fixed by #1854

Comments

@zenatti
Copy link

zenatti commented Jan 24, 2016

csscalc test always return false if I set usePrefixes false on modernizr 3.3.1

"el.style.cssText = prop + prefixes.join(value + prop);" doesn't check if Modernizr._config.usePrefixes is true or false.

@patrickkettner
Copy link
Member

I am not not able to replicate this

can you give a bit more information?

@zenatti
Copy link
Author

zenatti commented Jan 25, 2016

I have a build with usePrefixes:false

http://jsbin.com/leceyiyoti/edit?html,js,output

@patrickkettner
Copy link
Member

ah, im very sorry. I messed up the config when I made the bin :(

the issue is that we are joining the prefixes to get cssText, which is not working when prefixes is an empty array.

@ryanseddon @SlexAxton a hack fix would be to change prefixes from [] to ['','']. Otherwise we will need to reformat a handful of detects to check that config value and react accordingly, or change them from something like

var prop = 'width:';
var value = 'calc(10px);';
var el = createElement('a');

el.style.cssText = prop + prefixes.join(value + prop);

to something like

var prop = 'width:';
var value = 'calc(10px);';
var el = createElement('a');

el.style.cssText = prop + value + prefixes.join(value + prop);

the ['','']is the smallest amount of code, so I am leaning towards that. it just feels a bit dirty.

thoughts?

@SlexAxton
Copy link
Member

I don't see a huge need to byteshave here. I'd be happy to to just check for the length of 'prefixes' and do something different in the zero case. But I'm not against ['',''] if you feel strongly.

@patrickkettner
Copy link
Member

its a bit of a yak shaving moment, but the main reason I think about it is we would have to do the same check in every detect that uses prefixes.join method (about a dozen or so), or just short circuit the whole issue with a few chars.

if neither you or @ryanseddon are against the ['',''] way, i'll probably go that path with a comment

@ryanseddon
Copy link
Member

Nope not against it.

@SlexAxton
Copy link
Member

👍

On Tue, Jan 26, 2016 at 7:04 PM Ryan Seddon notifications@github.com
wrote:

Nope not against it.


Reply to this email directly or view it on GitHub
#1852 (comment)
.

patrickkettner added a commit to patrickkettner/Modernizr that referenced this issue Jan 27, 2016
patrickkettner added a commit to patrickkettner/Modernizr that referenced this issue Jan 27, 2016
patrickkettner added a commit to patrickkettner/Modernizr that referenced this issue Jan 27, 2016
patrickkettner added a commit to patrickkettner/Modernizr that referenced this issue Jan 27, 2016
@patrickkettner
Copy link
Member

@zenatti fixed in master, should be released as 3.4.0 in a few days

@zenatti
Copy link
Author

zenatti commented Jan 28, 2016

Thanks @patrickkettner !!

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

Successfully merging a pull request may close this issue.

4 participants