mootools / slick

Standalone CSS Selector Parser and Engine. An official MooTools project.
MIT License
150 stars 22 forks source link

Fix 'index' pseudo #64

Closed gpbmike closed 12 years ago

gpbmike commented 12 years ago

thanks

arian commented 12 years ago

Cool, it probably needs a small test, so it won't break in the future anymore.

I think https://github.com/mootools/slick/blob/master/SlickSpec/specs/select_nth-child.js would be the right file, right @subtleGradient?

GCheung55 commented 12 years ago

@ibolmo: I'm not against the solution; purely curious. But why?

ibolmo commented 12 years ago

It's more legible, but one can also claim it's better compression:

var a = String; // compiler would do this

// with String()
return this['pseudo:nth-child'](n, a(i + 1));

// without String()
return this['pseudo:nth-child'](n, '' + (i + 1)); // an extra 3 bytes (characters) :D

Ignore the whitespace.

GCheung55 commented 12 years ago

Well, I'd argue that it's not better compression because String isn't itself going to be compressed.


// With var created for String
var a = String; return this['pseudo:nth-child'](n, a(i + 1));

// With String
return this['pseudo:nth-child'](n, String(i + 1));

// Without String or var created for String
return this['pseudo:nth-child'](n, '' + (i + 1));

I'd go for the 2nd one though because of readability. :D

ibolmo commented 12 years ago

haha yeah it's a small suggestion. it's up to the slick people. if we follow the convention of String() instead of '' +' then overall we will save bytes.

I still think, though, that String(i + 1) is better understood than '' + (i + 1). The first is a "cast" while the latter is a coercion.

gpbmike commented 12 years ago

/unsubscribe

GCheung55 commented 12 years ago

@ibolmo Agreed. :)

arian commented 12 years ago

Looks like it's broken for 2 years :o https://github.com/mootools/slick/commit/b83da04

GCheung55 commented 12 years ago

I'd like to point out that this is still broken. the index that is past in is a string.

'1' + 1; // returns '11'

Spoke with @arian who said it should be

'' + (+index + 1);
arian commented 12 years ago

That was already fixed: https://github.com/mootools/slick/blob/master/Source/Slick.Finder.js#L815

sorbing commented 11 years ago

Why the fix is not in the master branch?

Bilge commented 10 years ago

I'm still experiencing this issue in 1.4.5. Not sure why it's closed.

Aybee commented 7 years ago

This still is buggy in 1.6.0. e.g. div:index(1) does not work. Only the 0 as index works.