Controlgroup: Correctly handle non-empty child class key #1713
Conversation
|
By analyzing the blame information on this pull request, we identified @arschmitz and @jzaefferer to be potential reviewers |
|
|
||
| } ); | ||
|
|
||
| } ); |
There was a problem hiding this comment.
This should not be its own file
3e0aa80 to
7e8d6e6
Compare
|
I don't see where you have addressed the vast majority of my comments and I see more changes with absolutely no explanation. |
7e8d6e6 to
35e51b1
Compare
|
Also failing jshint |
|
@arschmitz good that you told me to add the HTML into the markup! It exposed another problem: If you let the controlgroup instantiate the children, it always instantiates them with options, including classes options, but which are calculated without taking into account the default classes options stored in the prototype, because there is no instance on which to base those options. So, the prototype has to be the instance in that case, but the prototype cannot be gotten from the plugin. So, it has to be tacked onto the plugin in |
| .controlgroup() | ||
| .controlgroup( "instance" ); | ||
|
|
||
| assert.deepEqual( |
There was a problem hiding this comment.
Again why are you not just using hasClasses to ensure the element has the classes we want
There was a problem hiding this comment.
Because it's not the classes on the element that are wrong, but the value of the class key that's wrong.
.toggleClass() de-dupes when it applies classes, but the class key can look like "something-custom something-custom something-custom something-custom ui-corner-left".
There was a problem hiding this comment.
So, this is about the JS side, not the resulting markup.
There was a problem hiding this comment.
ok and why do we care if a user does that what does this break?
There was a problem hiding this comment.
The user doesn't do that. Controlgroup does that by the way it computes a value for the classes option.
There was a problem hiding this comment.
So, the user innocently sets $.ui.selectmenu.prototype.options.classes[ "ui-selectmenu-button-closed" ] to, say, "ui-shadow" and we proceed to mangle that value in every instance of a selectmenu that resides inside a controlgroup. Oh, and it's worse, because the length of the string increases with each refresh().
There was a problem hiding this comment.
I still dont think we need to test for that when the bug only existed as part of a greater bug that would have also failed with just a hasClasses check.
Alternatively if you really think this is important for you to check make it a separate test and just use matches on instance.options.classes[ 'whatever-key' ] and check the length of the class you added it should never be more then 1.
There was a problem hiding this comment.
Yeah, OK, I see what you're saying. I do believe that we should test both the class value with .hasClasses() and the class key value.
|
I'll add more comments. |
|
|
||
| // If the button is the child of a spinner ignore it | ||
| if ( widget === "button" && element.parent( ".ui-spinner" ).length ) { | ||
| return; | ||
| } | ||
| if ( instance ) { | ||
| options.classes = that._resolveClassesValues( options.classes, instance ); | ||
| if ( defaultInstance ) { |
There was a problem hiding this comment.
Instead of all this why not just
if ( !instance ) {
element[ widget ]();
}There was a problem hiding this comment.
OK, instantiate with default options first, and then call options. I can do that.
|
Please file an issue about whatever is being fixed. |
d6180d5 to
2c9c6e0
Compare
Fixes #14984 Closes jquerygh-1713
| .selectmenu( "option", "classes" )[ "ui-selectmenu-button-closed" ] | ||
| .match( /something-custom/ ) || [] ).length, 1, | ||
| "Class 'something-custom' appears exactly once in the second widget's class key value" ); | ||
| } ); |
There was a problem hiding this comment.
can we shorten all the class and id's here and in the html kind of excessively long
2c9c6e0 to
669d601
Compare
Fixes #14984 Closes jquerygh-1713
669d601 to
151bcfa
Compare
Fixes #14984 Closes jquerygh-1713
| QUnit.module( "Controlgroup: Non-empty class key", { | ||
| setup: function() { | ||
| this.classKey = $.ui.selectmenu.prototype.options.classes[ "ui-selectmenu-button-closed" ]; | ||
| $.ui.selectmenu.prototype.options.classes[ "ui-selectmenu-button-closed" ] = |
There was a problem hiding this comment.
why dont we just hard code a value here to simplify it really does not matter what we set here just that we set something non default
151bcfa to
36b81f5
Compare
Fixes #14984 Closes jquerygh-1713
| // We need to clone the default options for this type of widget to avoid | ||
| // polluting the variable options which has a wider scope than a single widget. | ||
| var instanceOptions = $.widget.extend( {}, options ); | ||
|
|
||
| // If the button is the child of a spinner ignore it | ||
| if ( widget === "button" && element.parent( ".ui-spinner" ).length ) { |
There was a problem hiding this comment.
I know you're not touching this block of code, but since you're modifying a bunch of code around it, can you just add a comment here that says "TODO: Find a more generic solution"? This is something @arschmitz and I discussed before, but we don't have any ideas for how to implement it.
Fixes #14984 Closes jquerygh-1713
36b81f5 to
928d3ee
Compare
|
@scottgonzalez I have addressed your review comments. |
|
thank you I will merge this shortly |
No description provided.