Some minor slider performance improvements #4458

Closed
wants to merge 1 commit into from

4 participants

@hpbuniat

I've made some minor performance-improvements to the slider-module, as the slider are currently not usable on my lg p500. With the patch applied, i'm able to set a specific value - but it still needs patience. I also looks a bit smoother on ipad2 (5.1.1), but i could be biased ;).

Changes:

  • removed repeating checks, if the control is an input or select
  • added a property cache to the slider, containing all static-properties which are used in refresh()
  • bypassed the value-alignment, when there is no step set, or the step is 1

I was wondering, if there is a remaining use-case for the trigger('change') in L394/409 ?

@jaspermdegroot
jQuery Foundation member

hi @hpbuniat

Thanks for your PR! Just a quick question... did you run the unit tests and did they all pass?

@hpbuniat

@uGoMobi, yes - on safari/ios 5.1.1, chrome 19/win7 & ff12/win7.
The chrome 19 had 1 failed test in the popup section, but i don't think that this related to these changes.

@johnbender

@hpbuniat

One quick note here. If you can it's helpful to pull them apart into discrete bits. For example this could be two pull requests, one for caching the cTypeIsInPut, and one the calculation changes/other.

At the very least it's good to split those into different commits so they can be dealt with individually. This will probably sit in the queue a while longer until someone gets the time to look over everything and how it interacts, unless you want to split it up.

Thanks for the pr.

@hpbuniat

@johnbender
Thanks for the hint. I'll mind this for upcoming pr's.

@toddparker

@johnbender - are you ok reviewing this PR as is? Definitely needs you to review since there is a fair amount of re-factoring here.

@johnbender

@hpbuniat

I should have reviewed this before I started refactoring parts of the slider to deal with memory management issues. But I'll take a look at what's here and what's in my branch and get back to you.

@johnbender

@hpbuniat

I did basically the exact same thing. I'm going to move over the ctype checks and give you author credits on the commit. Sorry for being so slow with this one. Totally my fault. :(

@johnbender johnbender added a commit that closed this pull request Aug 20, 2012
Hans-Peter Buniat Single check for slider input type Closes #4458
This commit is credited to Hans-Peter Buniat who put in the pull request
to do this centralization. We reached after the start of a slider refactor
and performance eval so the modifications have been added manually and credited.
08fe370
@johnbender

@hpbuniat

I've done a quick pass on the refresh perf and we're going to set up a perf regression suite in the next couple of releases. Stay tuned.

@hpbuniat

That's really good news.
I'm glad, this has been landed at last - that's new motivation to work on other issues :)

@toddparker

Please do, we'd appreciate your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment