search

tofsjonas / sortable

Vanilla JavaScript table sort
The Unlicense
429 stars 50 forks source link

Sorting doesn't work in some cases #30

Closed chatcoda closed 1 year ago

chatcoda commented 1 year ago

Good and concise work ! I noticed what could be considered a bug. Consider the following table:

Field
0
 
0
 

Sorting does not work because (at line 110 of sortable.js) isNaN(x - y) always returns false, since JavaScript interprets an empty string as zero. It could be fixed in several ways (regex, parseInt, trim + test empty string, maybe typeof, ...) This should do the trick: isNaN(parseInt(x) - parseInt(y))

tofsjonas commented 1 year ago

Yeah, I would call it a bug. Thanks for finding it and bringing it to my attention! 😊🙏

I like your solution, it's very pretty. I think x.length && y.length && !isNaN(x - y) would be slightly faster. 🤔

(I never bothered with real benchmarks, I'm too lazy)

Thoughts?

0hwow commented 1 year ago

I’ll give a try, bit busy :)

thanks

On 22 Nov 2022, at 10:32, Jonas Earendel @.***> wrote:

Yeah, I would call it a bug. Thanks for finding it and bringing it to my attention! 😊🙏

I like your solution, it's very pretty. I think x.length && y.length && !isNaN(x - y) would be slightly faster. 🤔

(I never bothered with real benchmarks, I'm too lazy)

Thoughts?

— Reply to this email directly, view it on GitHub https://github.com/tofsjonas/sortable/issues/30#issuecomment-1323365992, or unsubscribe https://github.com/notifications/unsubscribe-auth/AII3XK44TDETZJZRDGYR7GLWJSHLHANCNFSM6AAAAAASBXXNWA. You are receiving this because you are subscribed to this thread.