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

Improve performance and fix issues #728

Merged
merged 3 commits into from
Jan 21, 2018
Merged

Improve performance and fix issues #728

merged 3 commits into from
Jan 21, 2018

Conversation

SteveTheTechie
Copy link
Contributor

@SteveTheTechie SteveTheTechie commented Jan 17, 2018

  1. Add htmlButtonText option
  2. Add htmlOptionText option - Addresses Add option to use .html() instead of .text() to get option display and set checkbox span display for decoration purposes #696
  3. Use more local variables instead of frequently referencing widget properties/options.
  4. Use document.createElement() to more quickly create new DOM elements. Addresses I have a number of updates... #717 comment 1 and Opportunities for improved performance #724.
  5. Eliminate use of href="#" in header links. Add cursor: pointer to jquery.multiselect.css for these links.
  6. Move insertions to end of _create to limit browser reflowing lags.
  7. Use more ternary operators to eliminate if/then's where it makes sense.
  8. Create _updateCache() internal function... called from multiple places.
  9. Ditch use of $.isFunction. A simple typeof can be reliably used instead.
  10. Add check in update() to reposition menu if button height has changed from saved value and the menu is open. Addresses comment 5 in I have a number of updates... #717.
  11. Save the current button height in position() to enable checking for button height changes that require menu re-positioning. Addresses comment 5 in I have a number of updates... #717.

Tests Updated
12. Added test for namespace separation to core.js
13. Added selectedList:0 to tests in core.js to ensure that button text is correct for tests.
13. Fixed minWidth percentage test in options.js that was failing for non-Chrome browsers--this is likely due to floating point round off differences between browsers.

Files updated:
jquery.multiselect.js
jquery.multiselect.filter.js
jquery.multiselect.css
core.js
options.js

1. Add htmlButtonText option
2. Add htmlOptionText option 
3. Use more local variables instead of frequently referencing widget properties/options.
4. Use document.createElement() to more quickly create new DOM elements.
5. Eliminate use of href="#" in header links.  Add cursor: pointer to jquery.multiselect.css for these links.
6. Move insertions to end of _create to limit browser reflowing lags.
7. Use more ternary logic to eliminate if/then's where it makes sense.
8. Create _updateCache() internal function... called from multiple places.
9. Ditch use of $.isFunction.  A simple typeof can be reliably used instead.
10. Add check in update() to reposition menu if button height has changed from saved value and the menu is open.
11. Save the current button height in position().

Tests Updated
12. Added test for namespace separation to core.js
13. Added selectedList:0 to tests in core.js to ensure that button text is correct for tests.
13. Fixed minWidth percentage test in options.js that was failing for non-Chrome browsers.

Files updated:
jquery.multiselect.js
jquery.multiselect.filter.js
jquery.multiselect.css
core.js
options.js
@mlh758
Copy link
Collaborator

mlh758 commented Jan 17, 2018

Thanks for fixing that test, I'm not sure when it started failing but I noticed it on your first PR. I poked at it a couple of times but hadn't had time to sit down and investigate it thoroughly.

@SteveTheTechie
Copy link
Contributor Author

When I looked into the test, it appears that the difference is always 1 or less. You can get a difference of 1 if the parent div has an odd number of pixels width... This is why I thought that the floating point round off rationale is plausible. I just fixed the test by making a difference of 1 or less passable.

@mlh758
Copy link
Collaborator

mlh758 commented Jan 17, 2018

I used to always see a difference of less than one which is why I added that rounding operation.

On my mac I was seeing a difference of about 12 - 20. The issue was coming up because the element's parent width was changing substantially between lines of code in the test.

I'll pull down your changes and see if it is still fixed. If it's not, your changes are still an improvement for other browsers and the onus isn't on you to fix specs that have decide to die on some browsers so I'll still merge it.

@SteveTheTechie
Copy link
Contributor Author

Getting accurate reliable widths is one area where jQuery has historically had a number of bugs, so it may well be that this is a jQuery related issue. I would recommend keeping that in mind. In some cases, it may be necessary to ditch jQuery because of particular features that have had issues in the past.

https://www.google.com/search?q=jquery+width+bug&oq=jquery+width+bu&aqs=chrome.1.69i57j0l5.15910j0j7&sourceid=chrome&ie=UTF-8

@SteveTheTechie
Copy link
Contributor Author

SteveTheTechie commented Jan 17, 2018

The extra commits are just minor stuff I missed. Oops! Trying to keep the commits as close as possible to what I actually have in my editor. ;-)

@mlh758 mlh758 merged commit 105bb21 into ehynds:version3 Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants