All: Document classes option #257

jzaefferer · 2015-03-19T16:19:25Z

This document the default classes option and the _addClass method on $.Widget, and the classes option on accordion and draggable. There's a bunch of issues that need to be addressed. I'm including screenshots here to make previewing easier.

`classes` option on $.Widget:

Okay, but not very useful. Looking for suggestions how to improve this.

`_addClass` method:

Apparently <method> only supports a single <example> element, so the three descriptions are all mashed together, same for the code snippets. I think having multiple examples here is important enough to justify a change to grunt-jquery-content.
Multiple descriptions are rather bad, like "The element to add the classes to". Looking for suggestions for improving these.
I could make the reference to the classes option a link, but since that references this method, we end up with circular references without adding any value. Ignore?

Accordion `classes` option:

The Default has to be specified as an attribute, so it can't be formatted properly, so it looks really bad. Only alternative I can think of is to put the default into the <desc> element.
Is the description okay? If not, suggestions for improving it?
I've used a value of null for the example. I'm not sure if that is a good idea. See below for what I've used for draggable.

Draggable `classes` option:

The default value isn't quite as bad as for accordion, but still wraps over multiple lines.
Same description as for accordion - can/should we somehow share that?
In the example I'm pretending to add an extra class to the "draggable-handle". Is that good enough?

Once these issues are addressed, I can add the docs for _removeClass, _toggleClass and the remaining widgets.

arschmitz · 2015-03-19T16:58:03Z

A couple general things i noticed right off. In the widget documentation, it never mentions the concept of the option, which is to map structural classes to theme classes, that might help understanding. Second in the widget documentation the auto destroy functionality is never mentioned we want to include that for sure.

One thing that might help clear up the difference between keys, values, and extra along with their proper usage is to explain how we use them and the naming convention we use.

Keys - All classes for an individual widget are prefixed with the widget name ex: "ui-accordion" so any class starting with ui-accordion* is a key, its structural and specific to that widget these classes are always added no matter what.
Values - Theme classes that do things like add colors adjust border radius etc these classes are purely presentational and do not effect the function of the widget if missing. These classes are configurable
Extra - these classes are essential to the widget operation and or layout but are not specific to the widget. These classes are added no matter what.

On the example of getting and setting, we want to make sure that people understand if they pass an object, it will over ride the whole option, not just the values they pass. If we add an example of a deep option getter/ setter this should help clear that up.

I could make the reference to the classes option a link, but since that references this method, we end up with circular references without adding any value. Ignore?"

I think this still adds value because it avoids scrolling back and forth when your trying to understand the whole flow of the added classes stuff

The Default has to be specified as an attribute, so it can't be formatted properly, so it looks really bad. Only alternative I can think of is to put the default into the element.

Can we parse this and try to do some type checking? Kind of like what core does when you use .data on an attribute and it tries to figure out the type and return the correct type of data? @scottgonzalez is this at all possible to do with the xslt? if not i would change it to just say object and put it in the description I think.

Same description as for accordion - can/should we somehow share that?

Yes i think we should write a single description and share on every widget it will never change

jzaefferer · 2015-03-19T18:03:45Z

Updated, with lots of improvements. Take a look, I think I've addressed everything you commented on. Smashed examples are still a problem.

… one Ref jquery/api.jqueryui.com#257

jzaefferer · 2015-03-20T11:46:28Z

Once I got a review of my last update, jquery/grunt-jquery-content#63 is merged and published, and the full list of defaults is reviewed, I'll add the remaining methods and update the other widgets.

arschmitz · 2015-03-20T12:02:25Z

entries/jQuery.widget.xml

+					</ul>
+					<p>In addition to the structural and theme classes, there are also extra classes that are required for layout or other reasons and can't be removed. These are specified in the <code>extras</code> argument to <code>_addClass()</code>, but aren't part of this option.</p>
+					<p>As long as the <code>_addClass()</code>, <code>_removeClass()</code> and <code>_toggleClass()</code> methods are used, any added classes are automatically removed when the widget is destroyed. This can heavily simplify the implementation of custom <code>_destroy()</code> methods.</p>
+					<p class="warning">Setting the <code>classes</code> option will override all default properties. To only change specific values, use deep setters, e.g. <code>.option( "classes.ui-accordion-header", null )</code>.</p>


This is only AFTER init when using the getter / setter methods, on init its a deep extend. Might want to make that clear here.

… one Ref jquery/api.jqueryui.com#257 Closes #63

jzaefferer · 2015-03-21T15:54:57Z

@arschmitz I've addressed both your comments and fixed the grunt-jquery-content issue. If you could take another look and confirm my list ( #256 (comment) ), I can write the rest.

arschmitz · 2015-04-01T15:57:34Z

@jzaefferer have not had a chance to go through all of this yet but some things i wanted to put down so i don't forget

1.) we have added new ui-widgetname-* classes to some widgets so we want to make sure those get added to the theme sections where appropriate. For example selectmenu now has selectmenu-button-open and selectmenu-button-closed ( though it appears selectmenu has no theme section at all )

2.) While working on the markup structure tests @scottgonzalez and i had talked about if maybe we should also add the ui-widget-* classes to this section thoughts?.

scottgonzalez · 2015-04-03T17:57:25Z

entries/jQuery.widget.xml

@@ -192,6 +192,21 @@
 			<xi:include href="../includes/widget-option-disabled.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
 			<xi:include href="../includes/widget-option-hide.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
 			<xi:include href="../includes/widget-option-show.xml" xmlns:xi="http://www.w3.org/2003/XInclude"/>
+			<option name="classes" default="{}" example-value='{ "custom-header": "icon-warning" }'>
+				<desc>
+					<p>Extra classes to add to the widget. See the <a href="#method-_addClass"><code>_addClass()</code> method</a> for using this in custom widegts. Check out the documentation of individual widgets to see which classes they support.</p>


Typo: widegts

Perhaps provide more context upfront:

Additional (thematic) classes to add to the widget, in addition to the structural classes.

jzaefferer · 2015-04-09T16:32:19Z

Updated, addressing Scott's review and the last comment from Alex. Preview:

This depends on jquery/grunt-jquery-content#67 for the styling of <example> elements inside <option>. It also improves headers and description for the "Examples" section (same "Examples" header, but displays description as regular paragraph).

I'd like to avoid having to create custom examples for each widget, but that's currently blocked by jquery/grunt-jquery-content#66

Fixes #256

Also updates grunt-jquery-content to fix multiple examples for _addClass method.

scottgonzalez · 2015-04-10T13:28:36Z

I still think the default value should be the actual default value (i.e., none of the undefined values should exist). We should move the full structure up to the Theming section and reference it from the classes option description.

scottgonzalez · 2015-04-10T13:38:16Z

Let's update the classes option examples for jQuery.Widget so it shows setting the full value and a single value.

scottgonzalez · 2015-04-10T13:46:51Z

entries/jQuery.widget.xml

+				<desc>
+					Add classes to an element of the widget.
+					<p>This provides a hook for the user to add additional classes or replace default styling classes, through the <a href="#option-classes"><code>classes</code> option</a>.</p>
+					<p>It also provides automatic removal of these classes when a widget is destroyed, as long as you're using <code>_addClass()</code>, <code>_removeClass()</code> and <code>_toggleClass()</code> together. This can heavily simplify the implementation of custom <code>_destroy()</code> methods.</p>


Let's add links to the methods.

scottgonzalez · 2015-04-10T13:51:52Z

The missing links also apply to the other methods.

scottgonzalez · 2015-04-10T13:53:29Z

entries/jQuery.widget.xml

+					<desc>Additional classes to toggle, required for layout or other reasons. Unlike the <code>keys</code> argument, these aren't associated with any properties of the <code>classes</code> option. Just like <code>keys</code>, they will also be automatically removed when destroying the widget.</desc>
+				</argument>
+				<argument name="add" type="Boolean">
+					<desc>Indicates whether to add or remove the specified classes, where a boolean <code>true</code> indicates that classes should be added, a boolean <code>false</code> indicates that classes should be removed.</desc>


Even though the docs show that this is required, it's probably worth an explicit mention that unlike .toggleClass(), this method requires the boolean.

Thoughts on whether this should just be a regular sentence in the description or highlighted as a warning?

Personally i would say this is a big enough deviation from the potentially "expected" behavior that it merits a warning

I've added a warning to the method description.

…p setter

jzaefferer · 2015-04-13T14:53:33Z

Added three separate commits, that should address everything. For the add argument warning, I've added that to the <desc> of the _toggleClass() method.

scottgonzalez · 2015-04-14T19:15:27Z

The updates look good. I'll do another full review to see how everything looks at this point.

scottgonzalez · 2015-04-15T20:51:31Z

Did another round of review and everything looked good. I guess the next step is updating the individual widget docs for the remaining widgets?

arschmitz · 2015-04-15T21:04:08Z

I just finished reviewing again as well and everything looks good to me.

jzaefferer · 2015-04-19T18:55:15Z

Updated to include docs for all components. Turned out that theming information was missing or incomplete in many places. That was responsible for most of the effort.

Fixes gh-261

jzaefferer · 2015-04-20T18:28:54Z

Updated, addressed all of Scott's comments. I've changed the nesting for ui-selectee to go under ui-selectable, and both ui-sortable-handle and ui-sortable-placeholder under ui-sortable.

scottgonzalez · 2015-04-20T18:30:19Z

Looks good.

arschmitz · 2015-04-20T18:32:30Z

LGTM 👍

jzaefferer · 2015-04-20T18:40:35Z

Turns out I based this on master, instead of 1-12, and sent this PR against master as well. Will rebase against 1-12 and create a new PR with squashed commits.

jquerybot added the CLA: Valid label Mar 19, 2015

jzaefferer added a commit to jzaefferer/grunt-jquery-content that referenced this pull request Mar 20, 2015

API Sites: Loop over method examples instead of assuming there's just…

5236deb

… one Ref jquery/api.jqueryui.com#257

jzaefferer mentioned this pull request Mar 20, 2015

API Sites: Loop over method examples instead of assuming there's just one jquery/grunt-jquery-content#63

Closed

arschmitz reviewed Mar 20, 2015
View reviewed changes

jzaefferer added a commit to jquery/grunt-jquery-content that referenced this pull request Mar 21, 2015

API Sites: Loop over method examples instead of assuming there's just…

f841813

… one Ref jquery/api.jqueryui.com#257 Closes #63

scottgonzalez reviewed Apr 3, 2015
View reviewed changes

jzaefferer mentioned this pull request Apr 9, 2015

Consistent output for generated and manual examples jquery/grunt-jquery-content#65

Closed

jzaefferer added 5 commits April 9, 2015 22:45

All: Document classes option

fa08b58

Fixes #256

[tmp]: Extend jQuery.Widget, accordion and draggable classes docs

c7a5974

[tmp]: Update warning about classes option update; fix missing EOF

ffeb522

Also updates grunt-jquery-content to fix multiple examples for _addClass method.

[fix]: Many improvements

23eba79

[fix]: Assign getter to something

668c023

scottgonzalez reviewed Apr 10, 2015
View reviewed changes

jzaefferer added 3 commits April 13, 2015 16:04

[fix]: Add method and option links

4c9582f

[fix]: Document theme classes as part of the Theming options

e918e9a

[fix]: Custom example for widget classes option, show regular and dee…

3668d38

…p setter

[fix]: Add classes option for more widgets, update theming section

3d56060

jzaefferer added this to the 1.12 milestone Apr 19, 2015

[fix]: Document classes option and theming for remaining components

7ce635f

jzaefferer mentioned this pull request Apr 20, 2015

Document any deprecated options now replaced by classes option #261

Closed

jzaefferer added 2 commits April 20, 2015 18:16

Entries: Mark deprecated options

4d77652

Fixes gh-261

[fix]: Fix typos, bad nesting, sub( )menu, trailing comma

c1cb99d

jzaefferer changed the title ~~All: Document classes option [WIP]~~ All: Document classes option Apr 20, 2015

jzaefferer closed this Apr 20, 2015

jzaefferer mentioned this pull request Apr 20, 2015

All: Document classes option #262

Merged

jzaefferer deleted the classes-option branch April 20, 2015 18:49

All: Document classes option #257

All: Document classes option #257

Uh oh!

Conversation

jzaefferer commented Mar 19, 2015

classes option on $.Widget:

_addClass method:

Accordion classes option:

Draggable classes option:

Uh oh!

arschmitz commented Mar 19, 2015

Uh oh!

jzaefferer commented Mar 19, 2015

Uh oh!

jzaefferer commented Mar 20, 2015

Uh oh!

arschmitz Mar 20, 2015

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Mar 21, 2015

Uh oh!

arschmitz commented Apr 1, 2015

Uh oh!

scottgonzalez Apr 3, 2015

Choose a reason for hiding this comment

Uh oh!

scottgonzalez Apr 3, 2015

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Apr 9, 2015

Uh oh!

scottgonzalez commented Apr 10, 2015

Uh oh!

scottgonzalez commented Apr 10, 2015

Uh oh!

scottgonzalez Apr 10, 2015

Choose a reason for hiding this comment

Uh oh!

scottgonzalez commented Apr 10, 2015

Uh oh!

scottgonzalez Apr 10, 2015

Choose a reason for hiding this comment

Uh oh!

arschmitz Apr 13, 2015

Choose a reason for hiding this comment

Uh oh!

jzaefferer Apr 13, 2015

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Apr 13, 2015

Uh oh!

scottgonzalez commented Apr 14, 2015

Uh oh!

scottgonzalez commented Apr 15, 2015

Uh oh!

arschmitz commented Apr 15, 2015

Uh oh!

jzaefferer commented Apr 19, 2015

Uh oh!

jzaefferer commented Apr 20, 2015

Uh oh!

scottgonzalez commented Apr 20, 2015

Uh oh!

arschmitz commented Apr 20, 2015

Uh oh!

jzaefferer commented Apr 20, 2015

Uh oh!

Uh oh!

`classes` option on $.Widget:

`_addClass` method:

Accordion `classes` option:

Draggable `classes` option: