Code Review for the classes changes #790

petersendidit · 2012-10-24T16:53:53Z

No description provided.

scottgonzalez · 2013-04-12T11:44:51Z

ui/jquery.ui.widget.js

@@ -357,6 +357,19 @@ $.Widget.prototype = {
 		return this;
 	},

+	_classes: function( key ) {
+		var out = [],
+			parts = key.split(" "),


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

scottgonzalez · 2013-04-12T12:15:02Z

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

petersendidit · 2013-04-12T13:32:41Z

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

scottgonzalez · 2013-04-12T14:46:26Z

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" );


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 · 2013-04-17T14:15:00Z

Should be all set now.

petersendidit · 2013-04-25T14:54:07Z

ui/jquery.ui.menu.js

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


How do we handle the _classes stuff here?

petersendidit · 2013-04-25T14:54:25Z

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

petersendidit · 2014-06-02T15:58:14Z

ui/button.js

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


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

@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 · 2014-06-02T15:59:45Z

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

Fixes #7053

Deprecate dialogClass. Fixes #7053

Fixes #7053

petersendidit · 2014-08-24T01:50:11Z

ui/selectmenu.js

@@ -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 worked out nicely was able to resolve needing to change the class of the menu by using the classes option.

scottgonzalez · 2014-09-03T16:56:06Z

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

gabrielschulhof · 2014-09-04T22:23:41Z

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 · 2014-09-04T23:02:46Z

@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 · 2014-09-05T11:12:16Z

@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 · 2014-09-05T11:20:19Z

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 · 2014-09-05T12:02:56Z

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 · 2014-09-05T12:31:55Z

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

gabrielschulhof · 2014-09-05T12:32:44Z

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

gabrielschulhof · 2014-09-05T14:20:38Z

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 · 2014-09-05T14:46:31Z

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 · 2014-09-05T18:09:33Z

@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 · 2014-09-05T18:29:53Z

I worked past button on this implementation ...

gabrielschulhof · 2014-09-05T18:51:41Z

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

jzaefferer · 2014-10-22T16:09:35Z

Replaced by #1369

scottgonzalez reviewed Apr 12, 2013
View reviewed changes

petersendidit reviewed Apr 25, 2013
View reviewed changes

petersendidit reviewed Jun 2, 2014
View reviewed changes

Widget: add the _classes method.

cd43b3d

Fixes #7053

petersendidit force-pushed the classes_7053 branch from bffc556 to ee1890e Compare August 23, 2014 22:41

petersendidit added 11 commits August 23, 2014 20:40

Tabs: use the _classes method.

11528ff

Fixes #7053

Dialog: use the _classes method.

66a1347

Deprecate dialogClass. Fixes #7053

Accordion: use the _classes method.

d1cc07b

Fixes #7053

Autocomplete: use the _classes method.

b4e2f81

Fixes #7053

Slider: use the _classes method.

8746e4d

Fixes #7053

Spinner: use the _classes method.

bb7f6e9

Fixes #7053

Progressbar: use the _classes method.

6fca6fe

Fixes #7053

Menu: use the _classes method.

05fba95

Fixes #7053

Button: use the _classes method.

c3b8a80

Fixes #7053

Tooltip: use the _classes method.

171e6a2

Fixes #7053

Selectmenu: use the _classes method.

051c9c1

Fixes #7053

petersendidit force-pushed the classes_7053 branch from ee1890e to 051c9c1 Compare August 24, 2014 01:46

petersendidit reviewed Aug 24, 2014
View reviewed changes

arschmitz mentioned this pull request Oct 22, 2014

All: Classes Option #1369

Closed

jzaefferer closed this Oct 22, 2014

Code Review for the classes changes #790

Code Review for the classes changes #790

Uh oh!

Conversation

petersendidit commented Oct 24, 2012

Uh oh!

scottgonzalez Apr 12, 2013

Choose a reason for hiding this comment

Uh oh!

scottgonzalez commented Apr 12, 2013

Uh oh!

petersendidit commented Apr 12, 2013

Uh oh!

scottgonzalez Apr 12, 2013

Choose a reason for hiding this comment

Uh oh!

petersendidit commented Apr 17, 2013

Uh oh!

petersendidit Apr 25, 2013

Choose a reason for hiding this comment

Uh oh!

petersendidit commented Apr 25, 2013

Uh oh!

petersendidit Jun 2, 2014

Choose a reason for hiding this comment

Uh oh!

arschmitz Jun 2, 2014

Choose a reason for hiding this comment

Uh oh!

petersendidit commented Jun 2, 2014

Uh oh!

petersendidit Aug 24, 2014

Choose a reason for hiding this comment

Uh oh!

scottgonzalez commented Sep 3, 2014

Uh oh!

gabrielschulhof commented Sep 4, 2014

Uh oh!

scottgonzalez commented Sep 4, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

scottgonzalez commented Sep 5, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

scottgonzalez commented Sep 5, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

gabrielschulhof commented Sep 5, 2014

Uh oh!

jzaefferer commented Oct 22, 2014

Uh oh!

Uh oh!