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

Minor code improvements of lib.js #580

Closed
wants to merge 3 commits into from
Closed

Conversation

Mikhus
Copy link
Contributor

@Mikhus Mikhus commented Jun 18, 2013

  1. Added cross-browser support for Array.prototype.indexOf function
    It gives an ability to new browsers perform better on array
    search operations, and provides a fallback to the browsers
    which lacks this function support
  2. In all places declaration of vars moved to a single declaration blocks
    which is a good-practice
  3. Improved addClass function. Now it will not produce unnecessary spaces
  4. Improved removeClass function by using nativer indexOf function for
    browsers which do support it. Algorithm improved also, it now
    takes into account that className of an element could have same
    class name included several times, and as result removes all of them.

mstadnyk added 3 commits June 18, 2013 20:00
1. Added cross-browser support for Array.prototype.indexOf function
   It gives an ability to new browsers perform better on array
   search operations, and provides a fallback to the browsers
   which lacks this function support
2. In all places declaration of vars moved to a single declaration blocks
   which is a good-practice
3. Improved addClass function. Now it will not produce unnecessary spaces
4. Improved removeClass function by using nativer indexOf function for
   browsers which do support it. Algorithm improved also, it now
   takes into account that className of an element could have same
   class name included several times, and as result removes all of them.
@@ -1,15 +1,66 @@
var hasOwnProp = Object.prototype.hasOwnProperty;

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually tried adding Array indexOf before and it created issues in IE8 (#205). It's also a pretty hefty piece of code, so I don't think we want to add it back in since we've already worked around not having it.

@heff
Copy link
Member

heff commented Jun 24, 2013

The other changes look good. Could you remove the indexOf updates from the rest?

@Mikhus
Copy link
Contributor Author

Mikhus commented Jun 25, 2013

The problem is that some of other changes relying on Array.indexOf function :) Let me see what I can do in this case

@heff
Copy link
Member

heff commented Jun 26, 2013

Yeah, I think the other changes are still valuable.

@heff
Copy link
Member

heff commented Jul 18, 2013

@Mikhus do you think you'll be able to finish this off?

@Mikhus
Copy link
Contributor Author

Mikhus commented Jul 18, 2013

Sure, I will, in a few coming days
18.07.2013 21:25 ÐÏÌØÚÏ×ÁÔÅÌØ "Steve Heffernan" [email protected]
ÎÁÐÉÓÁÌ:

@Mikhus https://github.com/Mikhus do you think you'll be able to finish
this off?

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

@heff
Copy link
Member

heff commented Jul 18, 2013

Awesome, thanks.

On Jul 18, 2013, at 11:43 AM, Mykhailo Stadnyk [email protected] wrote:

Sure, I will, in a few coming days
18.07.2013 21:25 ÐÏÌØÚÏ×ÁÔÅÌØ "Steve Heffernan" [email protected]
ÎÁÐÉÓÁÌ:

@Mikhus https://github.com/Mikhus do you think you'll be able to finish
this off?

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


Reply to this email directly or view it on GitHub.

heff added a commit that referenced this pull request Aug 26, 2013
@heff heff closed this in 96f6c23 Aug 26, 2013
@heff
Copy link
Member

heff commented Aug 26, 2013

Got these changes pulled in. Thanks for pointing them out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: more info Please make enough detailed information is added to be actionable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants