Peformance 3 and Dimensions #735

SteveTheTechie · 2018-01-28T04:43:18Z

Closes out #569 and #731--these can be closed.
Partially addresses #717 and #727. (do not close yet)

Converted _makeOption to plain JS for speed. This is the last specific jQuery-to-plain-JS conversion that I am planning.
Added addInputNames option to allow opting out (false value) of creating name properties for inputs.
These names sometimes duplicate what is already in use for the native select and just cause more inputs to be serialized and posted to the server than is needed. (slower)
Added caching of button width value, menu width value, menu height value, and positioned status and mechanisms to invalidate the cached status for each. The caching minimizes the recalculation of each of these values when the menu is successively opened. (Just calculated on first open and re-used.)
Re-wrote _setMenuHeight:

a. Implement a new "size" keyword for the menuHeight option that sets the menu height based on the number of rows specified in the native select's "size" attribute. If "size" is specified, but no attribute is found on the native select, a default size value of 4 rows is used like is the default for native multiple selects.
b. Also, _setMenuHeight is optimized to execute the least loop iterations to determine the appropriate menu height to use.
c. Lastly, the menu height is limited to the available window.innerHeight value.

Changed from using self.element.parent().outerWidth() to using self.element.parent().innerWidth() in _getMinWidth() to fix errors in % width calculations.
Tweaked the css file to remove unnecessary tag qualifications that degrade layout efficiency.
Tweaked the css file to replace descendent selectors with child selectors where possible for improved layout efficiency.

1. Converted _makeOption to plain JS for speed. This is the last specific jQuery-to-plain-JS conversion that I am planning. 2. Added addInputNames option to allow opting out (false value) of creating name properties for inputs. These names sometimes duplicate what is already in use for the native select and just cause more inputs to be serialized and posted to the server than is needed. (slower) 3. Added caching of button width value, menu width value, menu height value, and positioned status and mechanisms to invalidate the cached status for each. The caching minimizes the recalculation of each of these values when the menu is successively opened. (Just calculated on first open and re-used.) 4. Re-wrote _setMenuHeight: - a. Implement a new "size" keyword for the menuHeight option that sets the menu height based on the number of rows specified in the native select's "size" attribute. If "size" is specified, but no attribute is found on the native select, a default size value of 4 rows is used like is the default for native multiple selects. - b. Also, _setMenuHeight is optimized to execute the least loop iterations to determine the appropriate menu height to use. - c. Lastly, the menu height is limited to the available window.innerHeight value. 5. Changed from using self.element.parent().outerWidth() to using self.element.parent().innerWidth() in _getMinWidth() to fix errors in % width calculations. 6. Tweaked the css file to remove unnecessary tag qualifications that degrade layout efficiency. 7. Tweaked the css file to replace descendent selectors with child selectors where possible for improved layout efficiency.

mlh758 · 2018-01-28T18:49:06Z

CHANGELOG.MD

@@ -0,0 +1,13 @@
+Summary of Code Changes for jQuery UI Multiselect Widget, Version 3.0.0:


Don't need a changelog file, we'll detail all the enhancements when we release the version 3 tag.

mlh758 · 2018-01-28T18:52:08Z

css/jquery.multiselect.css

@@ -1,25 +1,29 @@
+/*  References:  


I would prefer to stop including these reference comments. I was planning on going through and removing all of them from the old code once you were done merging changes.

I don't think they're really necessary and practices will change as web technologies evolve anyway.

mlh758

It looks like you added the auto method anyway. The algorithm you went with appears to be
checking the element to be appended to's width and using that. You're also appending the
menu to the body first, and then appending it to the appendEl. Does this cause excessive
reflowing of elements? Does grabbing the width on the body, and then putting it into another
element reliably work when the menu is inside a dialog?

mlh758 · 2018-01-28T19:21:51Z

src/jquery.multiselect.js

+   // default options
+   options: {
+      header: true,                       // (true | false) If true, the header is shown.
+      height: 175,                        // (int | 'size') Sets the height of the menu in pixels.  If 'size' is instead specified, the native select's size attribute is instead for height.


Your comment here is kind of confusing.

The 'size' setting just makes the height of the multiselect accommodate what is needed for the # of rows specified via the size setting of the native select. This is actually very useful... it is something I use a lot w/ my implemented version of my old fork in my web app.

I am totally open to less confusing wording.

It's mostly a grammar thing:

the native select's size attribute is instead for height.

could maybe be

the native select's size in rows attribute is used instead for height

mlh758 · 2018-01-28T19:22:44Z

src/jquery.multiselect.js

-      var elSelect = $element.get(0);      // This would be expected to be the underlying native select element.
+   // Performs the initial creation of the widget
+   _create: function() {
+      var $element = this.element.hide();                              // element property is a jQuery object per http://api.jqueryui.com/jQuery.widget/


I don't really see a need for comments describing what a jQuery object is within a jQuery widget. I think you deleted them before too.

mlh758 · 2018-01-28T19:22:58Z

src/jquery.multiselect.js

+   // Performs the initial creation of the widget
+   _create: function() {
+      var $element = this.element.hide();                              // element property is a jQuery object per http://api.jqueryui.com/jQuery.widget/
+      var elSelect = $element.get(0);                                  // This would be expected to be the underlying native select element.


It's expected to be... Is it not?

mlh758 · 2018-01-28T19:23:24Z

src/jquery.multiselect.js

-
-        this.$buttonlabel = $( document.createElement('span') )
+            .addClass('ui-multiselect ui-widget ui-state-default ui-corner-all' + (classes ? ' ' + classes : ''))
+            .attr({ 'type': 'button', 'title': elSelect.title, 'tabIndex': elSelect.tabIndex, 'id': elSelect.id ? elSelect.id  + '_ms' : null })


I would prefer you expanded it again, easier to read with the ternary hidden in there.

mlh758 · 2018-01-28T19:24:27Z

src/jquery.multiselect.js

+         if (inputAttribs[name] !== null)
+            input.setAttribute(name,inputAttribs[name]);
+      }
+      if ('dataset' in option) {


The dataset attribute isn't supported in IE 10, it only got added in 11 and for the time being I would prefer to maintain IE 10 support.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset

Thanks. Let me see if I can find a work around.

mlh758 · 2018-01-28T19:25:04Z

src/jquery.multiselect.js

+            $menu.appendTo('body').css({visibility:'hidden', width:'auto', display:'inline'}).find('ul').css('display', 'inline');
+            var autoWidth = $menu.width();
+            $menu.css({visibility:cssVisibility, width:cssWidth, display:cssDisplay}).find('ul').css('display', cssulDisplay).appendTo( appendEl );
+            console.log('AUTO WIDTH: ', cssWidth, autoWidth);


Delete the console statement.

Oops! Sorry about that. Will remove.

Also, I goofed as I did not intend to include this auto width functionality in this. I am still fiddling with it. I will remove it, also. (sorry)

mlh758 · 2018-01-28T19:25:32Z

src/jquery.multiselect.js

+      var width = self.options.menuWidth;
+
+      if (!width) {                                                   // Exact width not provided; determine appropriate width.
+         width = self._savedButtonWidth || self.$button.outerWidth();


You're in the recalc section here, and still using the saved value. Is that intentional?

It is not the same width... please examine it more closely.

It is referencing a button width in the setMenuWidth function. Since the button width was just likely already calculated, this just tries to use the recently recalculated cached value.

Ah, I see. That makes sense.

mlh758 · 2018-01-28T19:26:41Z

You're killing me with these whitespace changes...

Was it necessary to switch from this to self everywhere like that?

SteveTheTechie · 2018-01-28T21:39:31Z

My apologies. I apparently was a bit hasty in posting this, and I overlooked some things, and inadvertantly included some things I did not intend to include.

Also I will revert some of the things you mention.

- Removed extra self variables where possible. - Removed reference URLs (against my better judgement). - Used a simple alternative to dataset in _makeOptions (reference _getDataset() in (https://gist.github.com/ShirtlessKirk/6cdc2c32ddd97dc9c706)[https://gist.github.com/ShirtlessKirk/6cdc2c32ddd97dc9c706]) - Deleted spurious console.log statement - Ditched 'auto' setting for minWidth... not really done yet. - Deleted jQuery object comments.

SteveTheTechie · 2018-01-29T02:05:24Z

I think this should be ready for you to take another look. Thank you for your patience with me.

mlh758 · 2018-01-29T14:50:35Z

I'll try to get this code reviewed again today and test it out.

I saw this comment in my e-mail, although it doesn't appear to be on this chain anymore. It's worth discussing so I'm going to quote it in and reply anyway:

Michael, if you want other people to contribute, then you need to make sure that other people can quickly and easily understand the code and the foundations it is based on. If you are really committed to getting contributions from others, then it is unwise and insufficient for you to be the only one who really understands the code, Also, useful "understanding" is often more than a literal understanding, sometimes an understanding of the context in which the code was created is also helpful. This is why I find documenting references in comments to be helpful.

Useful code comments document document parameters, side effects, and purpose when it isn't clear. Your comment here is an example of a good comment. At first glance, it wouldn't necessarily be obvious to someone new why there are two of these. Reference comments require a contributor to follow a link, that may eventually be dead, and may no longer really apply, rather than actually explaining what the code below did that was so unusual that it required a comment instead of being clear on it's own what the purpose was.

Even the much vaunted jQuery's source code has a lot of comments in it. They also reference tickets and other websites in their code. Again, it ensures that the code can be easily picked up and updated by people other than the current maintainers.

It's not comments I take issue with, I'd love for this source code to eventually adhere to the Google style guide for documentation. There are also a couple of places in this source code where jQuery issues are referenced. Those are useful pieces of information and I have no intention of deleting them.

Frankly, what does it matter, anyway? If the code is minified (uglified) as it should be, then all the comments will be removed anyway from the actual running code.

I would prefer the comments that do need to be in place directly, and in plain language, explain the purpose (or side effects) of the code they are describing. Excessive comments can be just as detrimental to understanding as a lack of comments. Especially if they're used as a band-aid over confusing code that could be made less confusing.

mlh758 · 2018-01-29T15:44:00Z

I think this should be ready for you to take another look. Thank you for your patience with me.

You're contributing a lot of really helpful changes to the widget, so you're not testing my patience at all. I expect there to be some changes changes necessary on any code review that big.

mlh758 · 2018-01-29T17:39:58Z

Looks like this reduced the load time on the speed test from ~1 second on my work computer to .3 seconds. Impressive.

mlh758 reviewed Jan 28, 2018

View reviewed changes

mlh758 requested changes Jan 28, 2018

View reviewed changes

SteveTheTechie added 3 commits January 28, 2018 13:49

Remove references

2b16ce3

Deleted as requested

ed9840b

mlh758 approved these changes Jan 29, 2018

View reviewed changes

mlh758 merged commit 9837c2c into ehynds:version3 Jan 29, 2018

This was referenced Jan 29, 2018

Modifying multiselect list height in terms of rows, not pixels #569

Closed

More Performance Tweaks++ #731

Closed

		@@ -0,0 +1,13 @@
		Summary of Code Changes for jQuery UI Multiselect Widget, Version 3.0.0:

Peformance 3 and Dimensions #735

Peformance 3 and Dimensions #735

Uh oh!

Conversation

SteveTheTechie commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlh758 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlh758 Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveTheTechie Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlh758 commented Jan 28, 2018

Uh oh!

SteveTheTechie commented Jan 28, 2018

Uh oh!

SteveTheTechie commented Jan 29, 2018

Uh oh!

mlh758 commented Jan 29, 2018

Uh oh!

mlh758 commented Jan 29, 2018

Uh oh!

mlh758 commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SteveTheTechie commented Jan 28, 2018 •

edited

Loading

mlh758 Jan 29, 2018 •

edited

Loading

SteveTheTechie Jan 28, 2018 •

edited

Loading