Code Review for the classes changes #790

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
7 participants
@petersendidit
Member

petersendidit commented Oct 24, 2012

No description provided.

@scottgonzalez

View changes

ui/jquery.ui.widget.js
@@ -357,6 +357,19 @@ $.Widget.prototype = {
return this;
},
+ _classes: function( key ) {
+ var out = [],
+ parts = key.split(" "),

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

key.split( " " ) (we removed the exception for strings)

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

key.split( " " ) (we removed the exception for strings)

@scottgonzalez

View changes

ui/jquery.ui.widget.js
+ _classes: function( key ) {
+ var out = [],
+ parts = key.split(" "),
+ i;

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

undefined var at the top of the list

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

undefined var at the top of the list

@scottgonzalez

View changes

ui/jquery.ui.widget.js
+ parts = key.split(" "),
+ i;
+
+ for ( i = 0; i < parts.length; i++ ) {

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

@gibson042 @dmethvin @mikesherov I feel like I've asked this a million times, but what's the preferred looping method? Can someone who knows add it to the JS style guide?

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

@gibson042 @dmethvin @mikesherov I feel like I've asked this a million times, but what's the preferred looping method? Can someone who knows add it to the JS style guide?

This comment has been minimized.

@mikesherov

mikesherov Apr 12, 2013

Member

From the "good" examples:

// Good
var i = 0;

for ( ; i < 100; i++ ) {
    // expressions
}
@mikesherov

mikesherov Apr 12, 2013

Member

From the "good" examples:

// Good
var i = 0;

for ( ; i < 100; i++ ) {
    // expressions
}

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

I see, thanks. I was expecting a section on looping.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

I see, thanks. I was expecting a section on looping.

This comment has been minimized.

@gibson042

gibson042 Apr 12, 2013

Member

I'm pretty sure you're thinking about jquery/jquery#1046 (comment), which I'm loathe to put in the style guide because "whatever's smallest" (my driving factor) can change over time with the vagaries of minification and compression.

But at the moment, i = collection.length; while ( i-- ) and i = 0, len = collection.length; for ( ; i < len; i++ ) are almost always the best choices, with the latter being necessary for cases where forward iteration is required.

@gibson042

gibson042 Apr 12, 2013

Member

I'm pretty sure you're thinking about jquery/jquery#1046 (comment), which I'm loathe to put in the style guide because "whatever's smallest" (my driving factor) can change over time with the vagaries of minification and compression.

But at the moment, i = collection.length; while ( i-- ) and i = 0, len = collection.length; for ( ; i < len; i++ ) are almost always the best choices, with the latter being necessary for cases where forward iteration is required.

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

Yes, that's exactly what I was thinking of. I'll document that we prefer these two forms, but not say that they're absolute requirements.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

Yes, that's exactly what I was thinking of. I'll document that we prefer these two forms, but not say that they're absolute requirements.

@scottgonzalez

View changes

ui/jquery.ui.tabs.js
@@ -696,8 +707,7 @@ $.widget( "ui.tabs", {
$( this ).remove();
} else {
$( this )
- .removeClass( "ui-state-default ui-state-active ui-state-disabled " +
- "ui-corner-top ui-corner-bottom ui-widget-content ui-tabs-active ui-tabs-panel" )
+ .removeClass( that._classes( "ui-tabs-active ui-tab ui-tabs-panel" ) + " ui-helper-reset ui-helper-clearfix ui-widget-content ui-state-default ui-state-active ui-state-disabled" )

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

Split the string of static classes onto the next line.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

Split the string of static classes onto the next line.

@scottgonzalez

View changes

tests/unit/tabs/tabs_core.js
+
+ ok( ul.hasClass( "ui-tabs-nav" ), "list is .ui-tabs-nav" );
+ ok( ul.hasClass( "ui-helper-reset" ), "list is .ui-helper-reset" );
+ ok( ul.hasClass( "ui-helper-clearfix" ), "list is .ui-helper-clearfix" );

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

I don't think we should test for ui-helper-clearfix and ui-helper-reset. They're implementation details.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

I don't think we should test for ui-helper-clearfix and ui-helper-reset. They're implementation details.

@scottgonzalez

View changes

tests/unit/tabs/tabs_methods.js
+ domEqual( "#tabs2", function( done ) {
+ var element = $( "#tabs2" ).tabs({
+ load: function() {
+ setTimeout( function() {

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

No space before function(). (hooray for some exceptions?!?)

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

No space before function(). (hooray for some exceptions?!?)

@scottgonzalez

View changes

tests/unit/tabs/tabs_methods.js
+ element.tabs( "destroy" );
+ done();
+ start();
+ }, 1);

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

We tend to skip the duration parameter when using this as "run ASAP, but not now".

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

We tend to skip the duration parameter when using this as "run ASAP, but not now".

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Apr 12, 2013

Member

Overall this looks good. We should add tests for _classes() in widget_core.js.

Member

scottgonzalez commented Apr 12, 2013

Overall this looks good. We should add tests for _classes() in widget_core.js.

@petersendidit

This comment has been minimized.

Show comment
Hide comment
@petersendidit

petersendidit Apr 12, 2013

Member

Added a few tests for _classes() and fixed the problems noted in the diff.

Member

petersendidit commented Apr 12, 2013

Added a few tests for _classes() and fixed the problems noted in the diff.

@scottgonzalez

View changes

tests/unit/widget/widget_core.js
+ _create: function() {
+ equal( this._classes( "test" ), "test class1 class2" );
+ equal( this._classes( "test2" ), "test2 class2 class3" );
+ equal( this._classes( "test test2" ), "test2 class2 class3 test class1 class2" );

This comment has been minimized.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

Let's either not use dupes here or just check for existence of each class individually. We shouldn't have a test tied to the implementation detail of the order in which classes are produced and whether the result is deduped.

@scottgonzalez

scottgonzalez Apr 12, 2013

Member

Let's either not use dupes here or just check for existence of each class individually. We shouldn't have a test tied to the implementation detail of the order in which classes are produced and whether the result is deduped.

@petersendidit

This comment has been minimized.

Show comment
Hide comment
@petersendidit

petersendidit Apr 17, 2013

Member

Should be all set now.

Member

petersendidit commented Apr 17, 2013

Should be all set now.

ui/jquery.ui.menu.js
.attr( "role", "presentation" )
.children( "a" )
.uniqueId()
+ // TODO: Need to use _classes here
.addClass( "ui-corner-all" )

This comment has been minimized.

@petersendidit

petersendidit Apr 25, 2013

Member

How do we handle the _classes stuff here?

@petersendidit

petersendidit Apr 25, 2013

Member

How do we handle the _classes stuff here?

ui/jquery.ui.button.js
@@ -391,6 +403,7 @@ $.widget( "ui.buttonset", {
.map(function() {
return $( this ).button( "widget" )[ 0 ];
})
+ // TODO: need to figure out how to use _classes here
.removeClass( "ui-corner-all ui-corner-left ui-corner-right" )

This comment has been minimized.

@petersendidit

petersendidit Apr 25, 2013

Member

How do we handle the _classes stuff here?

@petersendidit

petersendidit Apr 25, 2013

Member

How do we handle the _classes stuff here?

@petersendidit

This comment has been minimized.

Show comment
Hide comment
@petersendidit

petersendidit Apr 25, 2013

Member

All the widgets should be using _classes now. Buttonset and Menu have some awkward places that we need to work out.

Member

petersendidit commented Apr 25, 2013

All the widgets should be using _classes now. Buttonset and Menu have some awkward places that we need to work out.

@@ -384,6 +397,7 @@ $.widget( "ui.buttonset", {
.map(function() {
return $( this ).button( "widget" )[ 0 ];
})
+ // TODO: need to figure out how to use _classes here

This comment has been minimized.

@petersendidit

petersendidit Jun 2, 2014

Member

Still have to figure out what to do here with these classes

@petersendidit

petersendidit Jun 2, 2014

Member

Still have to figure out what to do here with these classes

This comment has been minimized.

@arschmitz

arschmitz Jun 2, 2014

Member

@petersendidit I am currently working on the re-write of buttonset and button so this might not be an issue with the re-write. See the button-icon-span branch and the button page on the wiki.

@arschmitz

arschmitz Jun 2, 2014

Member

@petersendidit I am currently working on the re-write of buttonset and button so this might not be an issue with the re-write. See the button-icon-span branch and the button page on the wiki.

@petersendidit

This comment has been minimized.

Show comment
Hide comment
@petersendidit

petersendidit Jun 2, 2014

Member

Rebased the changes and fixed the conflicts. The problem that we had with classes in menu was removed by some refactoring that already happened in the menu widget, yay. Button still has one weird spot and selectmenu needs to be classes-afied

Member

petersendidit commented Jun 2, 2014

Rebased the changes and fixed the conflicts. The problem that we had with classes in menu was removed by some refactoring that already happened in the menu widget, yay. Button still has one weird spot and selectmenu needs to be classes-afied

@@ -141,6 +150,10 @@ return $.widget( "ui.selectmenu", {
// Initialize menu widget
this.menuInstance = this.menu
.menu({
+ // Adjust menu styles to dropdown
+ classes: {
+ "ui-menu": "ui-corner-bottom"

This comment has been minimized.

@petersendidit

petersendidit Aug 24, 2014

Member

This worked out nicely was able to resolve needing to change the class of the menu by using the classes option.

@petersendidit

petersendidit Aug 24, 2014

Member

This worked out nicely was able to resolve needing to change the class of the menu by using the classes option.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Sep 3, 2014

Member

Some places use null while others use "" when there are no additional classes to apply. Let's standardize on empty strings.

Member

scottgonzalez commented Sep 3, 2014

Some places use null while others use "" when there are no additional classes to apply. Let's standardize on empty strings.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 4, 2014

Contributor

I'm working on a way of allowing the classes option to be set via _setOptions(). In its current implementation it needs some buy-in from the widgets, but it lets you do things like this:

.dim {
  opacity: 0.5 !important;
}
$( ":ui-button:nth(1)" )
  .button( "option", "classes",
    $.extend( {},
      $( ":ui-button" ).button( "option", "classes" ),
      { "ui-button-icon-primary": "dim" } ) );

and

$( ":ui-button:nth(1)" )
  .button( "option", "classes",
    $.extend( {},
      $( ":ui-button" ).button( "option", "classes" ),
      { "ui-button-icon-primary": null } ) );

to turn dimming back off.

Despite this work, I am of the opinion that it's best not to promise that runtime changes to the classes option will be reflected immediately. If somebody changes the value, well, eventually, if they refresh the widget, or whatever, things will be reflected. That's fine, but don't expect us to update existing elements.

I'm looking into this to try and gauge how hairy it gets. If we can keep it relatively tidy, perhaps it's not such a big commitment to promise instant update via _setOption().

Contributor

gabrielschulhof commented Sep 4, 2014

I'm working on a way of allowing the classes option to be set via _setOptions(). In its current implementation it needs some buy-in from the widgets, but it lets you do things like this:

.dim {
  opacity: 0.5 !important;
}
$( ":ui-button:nth(1)" )
  .button( "option", "classes",
    $.extend( {},
      $( ":ui-button" ).button( "option", "classes" ),
      { "ui-button-icon-primary": "dim" } ) );

and

$( ":ui-button:nth(1)" )
  .button( "option", "classes",
    $.extend( {},
      $( ":ui-button" ).button( "option", "classes" ),
      { "ui-button-icon-primary": null } ) );

to turn dimming back off.

Despite this work, I am of the opinion that it's best not to promise that runtime changes to the classes option will be reflected immediately. If somebody changes the value, well, eventually, if they refresh the widget, or whatever, things will be reflected. That's fine, but don't expect us to update existing elements.

I'm looking into this to try and gauge how hairy it gets. If we can keep it relatively tidy, perhaps it's not such a big commitment to promise instant update via _setOption().

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Sep 4, 2014

Member

@gabrielschulhof The sane use is element.button( "option", "classes.ui-button-icon-primary", "dim" );

Regarding implementation, if we're going to create a registry of elements, the whole implementation should be changed to use a consistent method for create/refresh/update. Also, the registries need to be complete.

Member

scottgonzalez commented Sep 4, 2014

@gabrielschulhof The sane use is element.button( "option", "classes.ui-button-icon-primary", "dim" );

Regarding implementation, if we're going to create a registry of elements, the whole implementation should be changed to use a consistent method for create/refresh/update. Also, the registries need to be complete.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

@scottgonzalez I have re-implemented it without the need for storing references to elements. It's much better to use an extension point. I've made the process of resolving the various class keys extensible. autocomplete, button, dialog, selectmenu, and spinner needed an adjustment because they need to either selectively or unconditionally use this.widget() for the selector context rather than the default this.element.

I've also modified the selection process to not only look inside the context, but to also check the context itself for the presence of the class key.

In addition, I've modified some of the demos to loop over the demoed widgets and highlight the various structural elements, just to test the code. Please run the modified demo files to see the result!

Contributor

gabrielschulhof commented Sep 5, 2014

@scottgonzalez I have re-implemented it without the need for storing references to elements. It's much better to use an extension point. I've made the process of resolving the various class keys extensible. autocomplete, button, dialog, selectmenu, and spinner needed an adjustment because they need to either selectively or unconditionally use this.widget() for the selector context rather than the default this.element.

I've also modified the selection process to not only look inside the context, but to also check the context itself for the presence of the class key.

In addition, I've modified some of the demos to loop over the demoed widgets and highlight the various structural elements, just to test the code. Please run the modified demo files to see the result!

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

Hmmm ... I'm curious. If we use this.widget() for the default context we may be able to reduce the invasiveness of the code, because this.widget() evaluates to this.element after all. I'll make another branch.

Contributor

gabrielschulhof commented Sep 5, 2014

Hmmm ... I'm curious. If we use this.widget() for the default context we may be able to reduce the invasiveness of the code, because this.widget() evaluates to this.element after all. I'll make another branch.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

Yep. The version using this.widget() for the default context lets us touch two fewer files and it lets us return this._superApply( arguments ) more often. In this version, only autocomplete, dialog, and selectmenu need to override the default implementation of _elementsFromClassKey().

Contributor

gabrielschulhof commented Sep 5, 2014

Yep. The version using this.widget() for the default context lets us touch two fewer files and it lets us return this._superApply( arguments ) more often. In this version, only autocomplete, dialog, and selectmenu need to override the default implementation of _elementsFromClassKey().

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Sep 5, 2014

Member

I'm strongly against anything that does generic searching. It's a foot gun for nested widgets.

Member

scottgonzalez commented Sep 5, 2014

I'm strongly against anything that does generic searching. It's a foot gun for nested widgets.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

Yep, true. We're just discussing that with arschmitz.

Contributor

gabrielschulhof commented Sep 5, 2014

Yep, true. We're just discussing that with arschmitz.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

Well, it's less pretty if we avoid selectors, but it's still doable without storing the element a second time. IOW if we already have this.headers, we don't need to have a second variable this._elements[ "ui-accordion-header" ] or something like that if we use the extension point. The additional benefit of the extension point is that it's a function call, so there's no chance that the reference returned will be out of date, and we don't need to re-invent DOM insertion/removal to also store a second reference in a registry.

I've only worked this branch up to accordion, but I'll try to work through all the widgets.

Contributor

gabrielschulhof commented Sep 5, 2014

Well, it's less pretty if we avoid selectors, but it's still doable without storing the element a second time. IOW if we already have this.headers, we don't need to have a second variable this._elements[ "ui-accordion-header" ] or something like that if we use the extension point. The additional benefit of the extension point is that it's a function call, so there's no chance that the reference returned will be out of date, and we don't need to re-invent DOM insertion/removal to also store a second reference in a registry.

I've only worked this branch up to accordion, but I'll try to work through all the widgets.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Sep 5, 2014

Member

You should really just discuss the ideas instead of creating so many implementations. Are you aware that on every update, the full list of classes is going to be provided to _setOption() so you'd be calling this function several times? I also have no idea what this test-sequence file exists for, but I don't want to get into a discussion about that in this PR.

Member

scottgonzalez commented Sep 5, 2014

You should really just discuss the ideas instead of creating so many implementations. Are you aware that on every update, the full list of classes is going to be provided to _setOption() so you'd be calling this function several times? I also have no idea what this test-sequence file exists for, but I don't want to get into a discussion about that in this PR.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

@scottgonzalez Don't worry about the many implementations! They don't take long to create, and I get to know the code in the process. Don't worry about the test-sequence file either. Those branches are not intended for any kind of PR. I'm just playing around with various implementations, and I'd like to have code back up my comments. You also have an excellent eye for spotting problems a mile away looking at the code, so it's good for me to have code for you to look at :)

The idea behind the test-sequence.js file is that you open any of the demo files touched by the above-mentioned diff after pulling the corresponding branch and when you open it, the various structural elements of the widget being demoed become highlighted/de-highlighted every three seconds.

The code in test-sequence.js iterates over Object.keys( $.mobile[widgetBeingTested].prototype.options.classes ) and adds/removes the class "dim" to every one of those classes options via _setOptions(). The test sequence basically gives you a way to visually check whether the code is hitting all the structural elements. IOW, it makes sure the widget-specific implementation of _elementsFromClassKey() works.

I'm afraid I don't know which function you're referring to. AFAICT no function gets called several times. _setOptions() calls _setOption() exactly once, because only one option is being set: "classes". _setOption() in the widget factory iterates over all the classes in the new option value, and compares the value at each key value[ classKey ] to this.options.classes[ classKey ] and only calls _elementsFromClassKey() if the values differ.
So, if you only modify one of the values in the classes hash, _elementsFromClassKey() will only be called once.

Contributor

gabrielschulhof commented Sep 5, 2014

@scottgonzalez Don't worry about the many implementations! They don't take long to create, and I get to know the code in the process. Don't worry about the test-sequence file either. Those branches are not intended for any kind of PR. I'm just playing around with various implementations, and I'd like to have code back up my comments. You also have an excellent eye for spotting problems a mile away looking at the code, so it's good for me to have code for you to look at :)

The idea behind the test-sequence.js file is that you open any of the demo files touched by the above-mentioned diff after pulling the corresponding branch and when you open it, the various structural elements of the widget being demoed become highlighted/de-highlighted every three seconds.

The code in test-sequence.js iterates over Object.keys( $.mobile[widgetBeingTested].prototype.options.classes ) and adds/removes the class "dim" to every one of those classes options via _setOptions(). The test sequence basically gives you a way to visually check whether the code is hitting all the structural elements. IOW, it makes sure the widget-specific implementation of _elementsFromClassKey() works.

I'm afraid I don't know which function you're referring to. AFAICT no function gets called several times. _setOptions() calls _setOption() exactly once, because only one option is being set: "classes". _setOption() in the widget factory iterates over all the classes in the new option value, and compares the value at each key value[ classKey ] to this.options.classes[ classKey ] and only calls _elementsFromClassKey() if the values differ.
So, if you only modify one of the values in the classes hash, _elementsFromClassKey() will only be called once.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

I worked past button on this implementation ...

Contributor

gabrielschulhof commented Sep 5, 2014

I worked past button on this implementation ...

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 5, 2014

Contributor

Here's a video of the test sequence in action hitting the button classes.

Contributor

gabrielschulhof commented Sep 5, 2014

Here's a video of the test sequence in action hitting the button classes.

@arschmitz arschmitz referenced this pull request Oct 22, 2014

Closed

All: Classes Option #1369

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 22, 2014

Member

Replaced by #1369

Member

jzaefferer commented Oct 22, 2014

Replaced by #1369

@jzaefferer jzaefferer closed this Oct 22, 2014

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