Dimensions 2 #744

SteveTheTechie · 2018-02-06T03:03:47Z

Addresses #717 (partially) , #727, #737 (partially)

1. Updated sizing/dimensions code.
2. Added text wrapping option.
3. Using getBoundingClientRect()
4. Ditch globals
5. Update tests.
6. Update CSS
7. Add support for placeholder text. (via added commit)

Note that this includes the Qunit / tests update by @mlh758

1. Updated sizing/dimensions code. 2. Added text wrapping option. 3. Using getBoundingClientRect() 4. Ditch globals 5. Update tests. 6. Update CSS

SteveTheTechie · 2018-02-06T04:14:12Z

@mlh758 When you have had a chance to look at this, there are a few things I would to discuss related to defaults and default behavior for some of the new options.

mlh758 · 2018-02-06T14:13:00Z

Do you want to address those questions before merging this PR?

mlh758

After our discussion with code comments in an earlier PR, I'd prefer you at least stuck to the JSDoc stuff on your methods that return values.

mlh758 · 2018-02-06T14:15:58Z

tests/unit/options.js


+      width = "3in";
+      el.multiselect("option", "menuWidth", width).multiselect('refresh');
+      assert.equal(menu().parent().find(".ui-multiselect-menu").outerWidth(), 3 * 96.0, 'menuWidth supports strings suffixed with various units as well as integer px values');


Can you update the assert message here with the in unit?

mlh758 · 2018-02-06T14:16:19Z

tests/unit/options.js

      });

-      checkboxes = el.multiselect("widget").find(":checkbox");
+      var checkboxes = el.multiselect("widget").find(":checkbox");


Good catch adding all these vars.

Actually, this is a result of you updating Qunit. 👍

It has a new option you can set when running it to find all the global variables and flag them as test failures.

mlh758 · 2018-02-06T14:28:33Z

src/jquery.multiselect.js


      this.$buttonlabel = $( document.createElement('span') )
-            .html(options.noneSelectedText)
+            .html(options.noneSelectedText || $element[0].placeholder || 'Select options')


Do you need the third item in the || clauses? The description made it sound like if the option was null it would use the placeholder. If it's null and there's no placeholder I think I would prefer it do nothing or log a warning instead of having two places for the nonSelectedText option in the code.

That is fine. I can take that out.

mlh758 · 2018-02-06T14:31:27Z

src/jquery.multiselect.js

+     * @param {object} $checkboxes widget's container to append the built options to
     */
-    _buildOptionList: function($element, $checkboxContainer) {
+    _buildOptionList: function($element, $checkboxes) {


Now that the recursion is gone, we could probably use the instance variables of the widget instead of passing these parameters in. Don't need to fix it in this PR, just something I wanted to make a note of.

mlh758 · 2018-02-06T14:39:21Z

src/jquery.multiselect.js


      self._setButtonValue(value, isDefault);

+      if ( !/\bbutton\b/.test( options.wrapText ) )


For consistency I prefer that even single line if statements be wrapped in braces.

mlh758 · 2018-02-06T14:42:39Z

src/jquery.multiselect.js

+         var minimax = parsed.minimax;
+         width = minimax < 0 ? Math.max(width, pixels) : ( minimax > 0 ? Math.min(width, pixels) : pixels );
+      }
+      else // keywords


Same thing with the braces on all these.

Keywords : e.g. 'auto' The comment is just emphasizing that keyword options drop to the else.

mlh758 · 2018-02-06T15:01:52Z

src/jquery.multiselect.js

    /**
     * Toggles the checked state on options within the menu
     * Potentially scoped down to visible elements from filteredInputs
+     * Groups don't show up in the filter search right now, so both


I deleted this portion of the comment intentionally, it wasn't true.

Oh, did I revert the delete by accident?

mlh758 · 2018-02-06T15:02:27Z

src/jquery.multiselect.js

      // show the menu, maybe with a speed/effect combo
      // if there's an effect, assume jQuery UI is in use
-      $.fn.show.apply($menu, effect ? [ effect, speed ] : []);
+      if (effect)


Same thing with braces.

mlh758 · 2018-02-06T15:15:20Z

There seems to be an issue around refresh now. If you add an item after the initial instantiation of the widget, the menu doesn't resize or wrap tax to accommodate.

Before:

After:

mlh758 · 2018-02-06T15:47:02Z

I created a dialog demo in the same gist I had the speed test demo in. It looks like the width doesn't get calculated when the widget is in a dialog. Prior to this it would just wrap the text.

Edit: With a little more testing I found a bug in the base branch as well. Setting minWidth to 100% causes it to be a little bit too wide. Good illustration of what you are fixing in this branch.

SteveTheTechie · 2018-02-06T21:37:43Z

@mlh758

There seems to be an issue around refresh now. If you add an item after the initial instantiation of the widget, the menu doesn't resize or wrap tax to accommodate.

Look at the wrapText option... It should be set as 'button,header,menu' as the default. It looks like I goofed and forgot to set that before committing.

SteveTheTechie · 2018-02-07T00:43:51Z

@mlh758 Ok, made changes.

Can you please take another look at the layout issues you were seeing? I am hoping it was just a case of me previously forgetting to change the default value for an option back before committing.

SteveTheTechie · 2018-02-07T01:14:54Z

@mlh758 As to what I wanted to discuss... (this is long... sorry)

Height setting...

The height of the menu is either a specific value (w/ any of various units), 'size', or 'auto'. Size uses the native select size attribute to match the number of displayed rows. Auto attempts to maximize the height to show all items up to the limit of the available window height. The available window height is the constraint for all height settings--cannot set a height that exceeds that. 'Auto' is essentially the implicit fall-back in the code.

Could you please take another look at the Select2 code I excerpted in Determining the best width #727 related to auto width?
Width settings...

The width of the native select is captured in _create before hiding it.
Right now the general logic is that the width of the native select is used as a fall back for the button width, and the button width is used as a fall back for the menu width.
Thus, if you set buttonWidth = null and menuWidth = null, then both should get set to the same width as the native select.
If you set ">275px" for the button, it is treated as a minimum width for the button, meaning that if the select element width exceeds that, it is used--otherwise, it is 275px.

However, no min-width is set in the CSS like Select2. An alternative would be to just set the CSS min-width to 275px in this case and not set a hard width, but I am not sure it matters since the widget does not support dynamically resizing it, except by methods which can be coded to call for a resize. Also, I do not think that min-width is universally supported.
Also, what 'auto" should mean for the button width is a bit uncertain for me. If you kill the text wrapping for the button, it can grow in width, but whether that is practical is up for debate. There is no intrinsic width check for the button since it is native button element... just set width: auto for it.

If you set ">275px" for the menu, it is treated as a minimum width for the menu, meaning that if the button width exceeds that, it is used--otherwise, it is 275px.

However, again, no min-width is set in the CSS like Select2. An alternative would be to just set the CSS min-width to 275px in this case and not set a hard width, but I am not sure it matters since the widget does not support dynamically resizing it, except by methods which can be coded to call for a resize.
If you are going to add an option/item to the list, than the add option method is supposed to invalidate cached values, which causes the them to get recalc'd again the next open.

The parent of the native select is used as the "reference element" for % widths for both the button and menu widths.
If you set the menuWidth to 'auto', then the intrinsic / "shrink-wrapped" width of the header and checkbox container is determined and used to set a width for the menu. However, again, this is not set as a min-width like Select2 does, it is set as a hard width. Also, this width depends on whether text wrapping is on or off. (Typically, I think you would want it off for this case.)

If the menu width is set differently than the button via an explicit menuWidth option setting (vs null), there is currently nothing that goes back to force the button width to match it. If using groupColumns, the probably makes sense, but there is a question of whether some mechanism should exist to force the code to go back and change the button width after setting the menu width.
I think in most cases, you would just leave the menuWidth option as null to force a matched width, but for the case of menuWidth: 'auto', you could certainly end up with width that is wider than the button with no easy mechanism to fix it. It did occur to me that maybe buttonWidth: 'auto' & menuWidth: 'auto' would force the menu width code to go back and fix the button width to match. (but I am on the fence w/ that)

I tend to think that Select2 is trying to have a responsive widget that automatically adjusts its dimensions to fit its parent container. The question is whether we should be striving for that. I tend to think not, but I thought I would pose the question.

Again, sorry for the length...

Your thoughts?

mlh758 · 2018-02-07T14:14:12Z

Checked it out again, layout issues are fixed and that 100% issue I showed on the base branch is also fixed.

Edit: Your changes look good to merge now too.

mlh758 · 2018-02-07T14:51:19Z

However, no min-width is set in the CSS like Select2. An alternative would be to just set the CSS min-width to 275px in this case and not set a hard width, but I am not sure it matters since the widget does not support dynamically resizing it, except by methods which can be coded to call for a resize. Also, I do not think that min-width is universally supported.

min-width has good browser support for the basic pixel/percentage values. But yes, we only really resize at specifically coded moments, so I'm not really sure if that would give us any extra benefit. I think it might be worth adding a wiki example that shows people how to hook into window.resize and have the widget adjust its own dimensions.

Also, what 'auto" should mean for the button width is a bit uncertain for me. If you kill the text wrapping for the button, it can grow in width, but whether that is practical is up for debate. There is no intrinsic width check for the button since it is native button element... just set width: auto for it.

I think having the text wrapping on by default is the best way to go. It does seem to grow in auto mode now which, if someone wants that, is functional. I'm not sure on the practicality of it either but if it works and isn't the default behavior then I'm okay with it.

If the menu width is set differently than the button via an explicit menuWidth option setting (vs null), there is currently nothing that goes back to force the button width to match it.

To me that seems like the expected behavior of going out of your way to set the widths differently. I think the user could call multiselect({menuWidth: null}) to get it to resize after instantiation anyway. I would need to test that though to make sure.

It did occur to me that maybe buttonWidth: 'auto' & menuWidth: 'auto' would force the menu width code to go back and fix the button width to match. (but I am on the fence w/ that)

I don't think that's necessary since the default behavior is matching widths. If the user is changing these options it is presumably to no longer have them match.

I tend to think that Select2 is trying to have a responsive widget that automatically adjusts its dimensions to fit its parent container. The question is whether we should be striving for that. I tend to think not, but I thought I would pose the question.

I agree with you here. At some point, if too much of our behavior mimics theirs, it might be worth asking if it's really worth having a separate widget from theirs. It's probably our differences in behavior that will keep this project valuable, as long as those differences aren't degrading the user experience.

Could you please take another look at the Select2 code I excerpted in #727 related to auto width?

What is the takeaway I should be getting from that snippet? Their use of the css property?

SteveTheTechie · 2018-02-07T20:20:51Z

Could you please take another look at the Select2 code I excerpted in #727 related to auto width?

What is the takeaway I should be getting from that snippet? Their use of the css property?

Yes. I was just referencing that in my comments about min-width and whether I should maybe be setting it or any other CSS.

No matter... I think I have my answer from your other comments. 😉

I think you can merge this. I have a few closing comments I want to record "for posterity" in the related issue #727, but I will close that also.

SteveTheTechie added 3 commits February 5, 2018 18:53

Updating to QUnit 2.5, some spec formatting

24f14a8

Dimensions 2

8414401

1. Updated sizing/dimensions code. 2. Added text wrapping option. 3. Using getBoundingClientRect() 4. Ditch globals 5. Update tests. 6. Update CSS

Merge branch 'version3' into version3

730c37f

SteveTheTechie changed the title ~~Dimensions 3~~ Dimensions 2 Feb 6, 2018

Add support for native select placeholder text

25e3591

mlh758 requested changes Feb 6, 2018

View reviewed changes

SteveTheTechie added 2 commits February 6, 2018 16:39

Requested changes

529a6df

Changes requested + buttonWidth vs minWidth

5b514cd

Fix an oversight

812694e

mlh758 approved these changes Feb 7, 2018

View reviewed changes

mlh758 merged commit 7002ad8 into ehynds:version3 Feb 7, 2018

This was referenced Feb 9, 2018

maxWidth option? #412

Closed

Wrap Text of each Select Option #365

Closed

minWidth auto ends up with a width calculation that's a bit too short. #544

Closed


		self._setButtonValue(value, isDefault);

		if ( !/\bbutton\b/.test( options.wrapText ) )

Dimensions 2 #744

Dimensions 2 #744

Uh oh!

Conversation

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

Uh oh!

Uh oh!

SteveTheTechie commented Feb 6, 2018

Uh oh!

mlh758 commented Feb 6, 2018

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!

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!

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 commented Feb 6, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

SteveTheTechie commented Feb 7, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mlh758 commented Feb 7, 2018

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SteveTheTechie commented Feb 6, 2018 •

edited

Loading

mlh758 commented Feb 6, 2018 •

edited

Loading

SteveTheTechie commented Feb 6, 2018 •

edited

Loading

SteveTheTechie commented Feb 7, 2018 •

edited

Loading

mlh758 commented Feb 7, 2018 •

edited

Loading

SteveTheTechie commented Feb 7, 2018 •

edited

Loading