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

Removed dashes from property names corresponding to feature detects. #782

Closed
wants to merge 3 commits into
base: 3pre
from

Conversation

Projects
None yet
5 participants
@ghost

ghost commented Jan 8, 2013

The dashes were preventing these detects from being queried like every other detect. For example, before this PR one could use

if (!Modernizr.fontface) { /* (...) */ }

to test for @font-face support, but one must use

if (!Modernizr["display-table"]) { /* (...) */ }

to achieve the same for display: table.

bmcustodio
Removed dashes from property names corresponding to feature detects.
The dashes were preventing these detects from being queried like every other detect. For example, before this PR one could use

```
if (!Modernizr.fontface) { /* (...) */ }
```

to test for `@font-face` support, but one must use

```
if (!Modernizr["display-table"]) { /* (...) */ }
```

to achieve the same for `display: table`.
@paulirish

This comment has been minimized.

Member

paulirish commented Jan 8, 2013

Excellent thank you

bmcustodio
Removed dashes from property names corresponding to feature detects.
The dashes were preventing these detects from being queried like every other detect. For example, before this PR one could use

```
if (!Modernizr.fontface) { /* (...) */ }
```

to test for `@font-face` support, but one must use

```
if (!Modernizr["display-table"]) { /* (...) */ }
```

to achieve the same for `display: table`.
@KuraFire

This comment has been minimized.

Member

KuraFire commented Jan 8, 2013

@paulirish Don't we want to provide aliases for their old names for the first 1-2 releases following this change?

@KuraFire

This comment has been minimized.

Member

KuraFire commented Jan 8, 2013

Addendum: Right, I made a comment about this in the original thread but it didn't really specify whether I supported breaking these instantly in 3.0 or by way of a compatibility alias 1-2 versions later.

I guess since these aren't the most common features, and since 3.0 is a major milestone with tons of structural changes, I am okay with an instant backwards-incompatible change. @paulirish @SlexAxton @ryanseddon @aFarkas — unless any of you have strong thoughts on this?

@SlexAxton

This comment has been minimized.

Member

SlexAxton commented Jan 8, 2013

Dunno about whether we WANT aliases, but I built in first-class support for them in v3, so it'd be pretty easy...

https://github.com/Modernizr/Modernizr/blob/3pre/feature-detects/blob.js#L12

@KuraFire

This comment has been minimized.

Member

KuraFire commented Jan 8, 2013

<3 slex

@paulirish

This comment has been minimized.

Member

paulirish commented Jan 9, 2013

These are all pretty uncommon detects, so I'm okay with a break.

Anyone else watching think we should alias for a transition period?

@SlexAxton

This comment has been minimized.

Member

SlexAxton commented Jan 9, 2013

I'm probably in favor of aliasing anything thatd be a commonly used css class.

@ghost

This comment has been minimized.

ghost commented Jan 9, 2013

Ok, so just let me know what your decision about this is and if needed I'll update the PR to include the aliases and to amend the test (since it will fail as it is in that case.) ;-)

@paulirish

This comment has been minimized.

Member

paulirish commented Jan 9, 2013

Yes, let's update the PR with aliases.

Here is the details I know of for how to add an alias:
#713 (comment)

On Tue, Jan 8, 2013 at 11:07 PM, Bruno M. Custódio <notifications@github.com

wrote:

Ok, so just let me know what your decision about this is and if needed I'll
update the PR to include the aliases and to amend the test (since it will
fail as it is in that case.) ;-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-12033929.

@KuraFire

This comment has been minimized.

Member

KuraFire commented Jan 9, 2013

Yeah, I suggest we do aliases for these properties, but for only one .X version. They’re all quite uncommon, I don’t think we need to keep them around for longer than that.

@ghost

This comment has been minimized.

ghost commented Jan 10, 2013

@paulirish @SlexAxton @ryanseddon @aFarkas
Done. I've added the old names as aliases for every name that got changed, and modified the test so that it fails if it finds a dashed name which doesn't have an undashed alias defined for it.

@riddla

This comment has been minimized.

riddla commented Jan 10, 2013

Just stumbled on this topic when fiddling with a test suite for some browser detections. I was wondering why my test for nasty-browser-bug wasn't attached as a property to the Modernizr object.
One beer later i finally realized that a property with dashes might be quite 'uncommon' on a JavaScript object ;)

However, as the API documentation is quiet about the form of the 'str' parameter within addTest (i know, a string is a string is a string): others might follow my line of thought. Can (or should) there be a foolproof note about this? Or a best practice for naming tests?

PS: As a non-native english speaker/writer: is "line of thought" common? "way of thinking"?

@paulirish

This comment has been minimized.

Member

paulirish commented Jan 10, 2013

"line of thinking" would be best here but all the above are totally
readable. :)

Yes I'll augment the docs on this. good call.

On Thu, Jan 10, 2013 at 1:14 PM, Volker Rose notifications@github.comwrote:

Just stumbled on this topic when fiddling with a test suite for some
browser detections. I was wondering why my test for nasty-browser-bugwasn't attached as a property to the Modernizr object.
One beer later i finally realized that a property with dashes might be
quite 'uncommon' on a JavaScript object ;)

However, as the API documentation is quiet about the form of the 'str'
parameter within addTest (i know, a string is a string is a string):
others might follow my line of thought. Can (or should) there be a
foolproof note about this? Or a best practice for naming tests?

PS: As a non-native english speaker/writer: is "line of thought" common?
"way of thinking"?


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-12118990.

@paulirish

This comment has been minimized.

Member

paulirish commented Jan 10, 2013

Modernizr/the-old-modernizr.com@ccda4c2...14f895a

On Thu, Jan 10, 2013 at 3:36 PM, Paul Irish paul.irish@gmail.com wrote:

"line of thinking" would be best here but all the above are totally
readable. :)

Yes I'll augment the docs on this. good call.

On Thu, Jan 10, 2013 at 1:14 PM, Volker Rose notifications@github.comwrote:

Just stumbled on this topic when fiddling with a test suite for some
browser detections. I was wondering why my test for nasty-browser-bugwasn't attached as a property to the Modernizr object.
One beer later i finally realized that a property with dashes might be
quite 'uncommon' on a JavaScript object ;)

However, as the API documentation is quiet about the form of the 'str'
parameter within addTest (i know, a string is a string is a string):
others might follow my line of thought. Can (or should) there be a
foolproof note about this? Or a best practice for naming tests?

PS: As a non-native english speaker/writer: is "line of thought" common?
"way of thinking"?


Reply to this email directly or view it on GitHubhttps://github.com//pull/782#issuecomment-12118990.

@riddla

This comment has been minimized.

riddla commented Jan 11, 2013

Man, your quick. Good stuff, thank you.

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