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

Add feature detect for nth-child() #685

Merged
merged 18 commits into from Jul 1, 2013
Merged

Conversation

@emilchristensen
Copy link
Contributor

@emilchristensen emilchristensen commented Sep 7, 2012

Reference: http://reference.sitepoint.com/css/pseudoclass-nthchild

Tested to work in:
Internet Explorer 7 (false), 8 (false), 9 (true)
Chrome 7 (true), 21 (true)
Firefox 3.6 (true), 14 (true)
Safari 5.2 (true), 6 (true)
Opera 9.0 build 3447 (false), 12 (true)

Known issues in:
Safari 3.1, 3.2.2 (false negatives)

return Modernizr.testStyles("#modernizr div {width:1px} #modernizr div:nth-child(2n) {width:2px;}", function (elem) {
var elems = elem.getElementsByTagName("div"),
test = true;
for (i=0; i<5; i++) {

This comment has been minimized.

@sindresorhus

sindresorhus Sep 7, 2012
Contributor

You're leaking the i

// nth-child pseudo selector
Modernizr.addTest('nthchild', function(){

return Modernizr.testStyles("#modernizr div {width:1px} #modernizr div:nth-child(2n) {width:2px;}", function (elem) {

This comment has been minimized.

@sindresorhus

sindresorhus Sep 7, 2012
Contributor

single-quotes


return Modernizr.testStyles("#modernizr div {width:1px} #modernizr div:nth-child(2n) {width:2px;}", function (elem) {
var elems = elem.getElementsByTagName("div"),
test = true;

This comment has been minimized.

@sindresorhus

sindresorhus Sep 7, 2012
Contributor

indent

@sindresorhus
Copy link
Contributor

@sindresorhus sindresorhus commented Sep 7, 2012

Please add a description to the PR about how the test works.

var elems = elem.getElementsByTagName('div'),
test = true;

for (var i=0; i<5; i++) {

This comment has been minimized.

@sindresorhus

sindresorhus Jan 24, 2013
Contributor

for (var i = 0; i < 5; i++) {

test = true;

for (var i=0; i<5; i++) {
test = test && elems[i].offsetWidth == i%2+1;

This comment has been minimized.

@sindresorhus

sindresorhus Jan 24, 2013
Contributor

=== i % 2 + 1

@@ -0,0 +1,15 @@
// nth-child pseudo selector
Modernizr.addTest('nthchild', function(){

This comment has been minimized.

@sindresorhus

sindresorhus Jan 24, 2013
Contributor

function() {

@sindresorhus
Copy link
Contributor

@sindresorhus sindresorhus commented Jan 24, 2013

@alexslade
Copy link

@alexslade alexslade commented Feb 14, 2013

👍

@bradgreens
Copy link

@bradgreens bradgreens commented Jun 25, 2013

+1

@stucox
Copy link
Member

@stucox stucox commented Jun 26, 2013

This needs updating to the v3 format before we can accept it.

@emilchristensen are you able to do this, or does someone else fancy volunteering?

@emilchristensen
Copy link
Contributor Author

@emilchristensen emilchristensen commented Jun 26, 2013

I'll update it to v3 asap

@stucox
Copy link
Member

@stucox stucox commented Jun 26, 2013

Great, thanks.

@emilchristensen
Copy link
Contributor Author

@emilchristensen emilchristensen commented Jun 28, 2013

Alright, this is updated and should adhere to the v3 principle and former comments in this thread.

@emilchristensen
Copy link
Contributor Author

@emilchristensen emilchristensen commented Jul 1, 2013

Need me to squash it before merge or are we good to go?

- Moved 'how it works' out of DOC block (this is for user documentation, rather than maintainer/implementation documentation – I've also clarified this on [the wiki](https://github.com/Modernizr/Modernizr/wiki/Authoring-a-v3-test))
- Turned `"authors"` and `"warnings"` fields into arrays
- Corrected `"property"` field to match name used in `Modernizr.addTest()` (these must be the same)
@stucox
Copy link
Member

@stucox stucox commented Jul 1, 2013

Thanks. I've raised a PR on your fork of the repo with a couple of changes to the metadata/DOC blocks.

If you're happy with that, could you merge that in, squash the commits and push again? Then we'll pull it in.

Thanks :-)

@emilchristensen
Copy link
Contributor Author

@emilchristensen emilchristensen commented Jul 1, 2013

I couldn't rebase n' squash all the way back I'm afraid. I'm guessing too big difference in the structure of how the project looked back then. I've been able to squash back to 1fe1e83. Do you want me to push that? Or do you have an idea on how to fix the problems (I'm thinking it's something with the files getting added in different places)

@stucox
Copy link
Member

@stucox stucox commented Jul 1, 2013

Yeah if you could push that, that'll be fine – thanks.

stucox pushed a commit that referenced this pull request Jul 1, 2013
Add feature detect for nth-child()
@stucox stucox merged commit 97aaacd into Modernizr:master Jul 1, 2013
1 check passed
1 check passed
@SlexAxton
default The Travis CI build passed
Details
@stucox
Copy link
Member

@stucox stucox commented Jul 1, 2013

Nice one, thanks @emilchristensen!

patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this pull request Feb 22, 2015
Add feature detect for nth-child()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants