fix Rainbow text for Forums

stepmania / stepmania-site

StepMania's Website, forums, etc.

https://www.stepmania.com/

19 stars 7 forks source link

fix Rainbow text for Forums #58

Closed quietly-turning closed 9 years ago

quietly-turning commented 9 years ago

If a single page in a thread contained multiple instances of the rainbow markup, the existing JavaScript would make the text of each match the last instance found. This behavior exists in the original implementation ( https://github.com/xoxco/rainbow-text ) as can be seen here: http://jsfiddle.net/nzv2L259/

This is a simple fix: iterate over all instances of p.rainbow found and apply the rainbow() function to each individually.

There are valid arguments that doing this with a plain JavaScript for loop is faster; I'm not sure it's really worth worrying about at this time.

SoonDead commented 9 years ago

Nice catch. I have submitted a pull request to xoxo/rainbow-text where this problem is handled internally.

Let's see if the plugin author accepts the pull, and if not merge this.

shakesoda commented 9 years ago

Watching the upstream PR for this.

quietly-turning commented 9 years ago

I like your fix more, @SoonDead. It didn't occur to me until seeing your pull request that my own needlessly introduced polynomial time to an originally linear problem. Also, I was previously unfamiliar with jQuery.extend() so thank you!

shakesoda commented 9 years ago

Doesn't seem like upstream is around to take the patch. @SoonDead, care to make your fix into a PR here to replace this one?

SoonDead commented 9 years ago

@dguzek I don't think it performs significantly better than your solution. If you think it through there are no unnecessary iterations over elements either way:

In your solution the only unnecessary overhead is calling .each() on arrays with only one element which is an O(1) operation in itself.
Therefore the whole solution is still in just a linear relationship with the count of elements. So is mine.

I created the pull request at the original project, because it's clearly a bug in the original plugin, and it should be fixed there.

So all in all @shakesoda you are safe to merge @dguzek's PR, but I'll recreate my pull request here too, flip a coin merge one, reject the other :) It's PR #60

shakesoda commented 9 years ago

Thanks! I'm merging #60 because I think it's the more elegant solution, not due to performance. :)