* More API documentation. #1254

Merged
merged 1 commit into from Apr 10, 2012

Conversation

Projects
None yet
4 participants
Owner

fmpwizard commented Apr 10, 2012

* Mostly net.liftweb.http.js.

@fmpwizard fmpwizard * More API documentation.
** Mostly net.liftweb.http.js.*
fcae069
Member

pr1001 commented Apr 10, 2012

You also removed a bunch of imports... I assume the tests still pass? Looks good!

Owner

fmpwizard commented Apr 10, 2012

Ah yes, I forgot about the imports I removed.
All test passed

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...la/net/liftweb/http/js/extcore/ExtCoreArtifacts.scala
object ExtCoreArtifacts extends JSArtifacts {
+ /**
+ * Toggles between current JS object and the object denominated by id
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

This actually toggles the visibility of the object denominated by id (specifically, I believe it switches between display: none and whatever the default display value is for the particular Element).

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...la/net/liftweb/http/js/extcore/ExtCoreArtifacts.scala
def show(id: String) = new JsExp {
def toJsCmd = "Ext.fly(" + id.encJs + ").show()"
}
+ /**
+ * Shows the element denoinated by id and puts the focus on it
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

“denominated”

Member

pr1001 commented Apr 10, 2012

Cool, always great to reduce imports. Then it looks good to go!

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...la/net/liftweb/http/js/extcore/ExtCoreArtifacts.scala
def serialize(id: String) = new JsExp {
def toJsCmd = "Ext.Ajax.serializeForm(" + id.encJs + ")"
}
+ /**
+ * Replaces the content of the node with the provided id with the markup given by content
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

The phrasing here is a tad confusing. Perhaps “Replaces the content of the node denominated by the id with the markup given by content”, which will also align it with the verbiage on previous docs.

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...la/net/liftweb/http/js/extcore/ExtCoreArtifacts.scala
def setHtml(id: String, xml: NodeSeq): JsCmd = new JsCmd {
def toJsCmd = fixHtmlCmdFunc(id, xml){s => "try { Ext.fly(" + id.encJs + ").dom.innerHTML = " + s + "; } catch (e) {}"}
}
+ /**
+ * Sets the JavScript that will be executed when document is ready
+ * for processing
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Maybe it would be more accurate to say that this adds some JavaScript to be executed when the DOM is ready for processing? It seems like “set” implies that it would replace a previous setting done by a call to this.

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...la/net/liftweb/http/js/extcore/ExtCoreArtifacts.scala
def onLoad(cmd: JsCmd): JsCmd = new JsCmd {
def toJsCmd = "Ext.onReady(function() {" + cmd.toJsCmd + "})"
}
+ /**
+ * Fades out the element having the provided id, by waiting
+ * for the given duration and fades out during fadeTime
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

For grammatical purposes, “waiting for” should match with “fading out” at the end of the sentence.

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...la/net/liftweb/http/js/extcore/ExtCoreArtifacts.scala
def comet(data: AjaxInfo): String = {
"Ext.Ajax.request(" + toJson(data, LiftRules.cometServer(), LiftRules.calcCometPath) + ");"
}
+ /**
+ * Trabsforms a JSON object intoits string representation
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

“Transforms”* “into its”*

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...cala/net/liftweb/http/js/jquery/JQueryArtifacts.scala
def comet(data: AjaxInfo): String = {
"jQuery.ajax(" + toJson(data, LiftRules.cometServer(), LiftRules.calcCometPath) + ");"
}
+ /**
+ * Trabsforms a JSON object intoits string representation
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Basically seems like the same remarks apply as above in this file :)

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -159,10 +159,16 @@ object JqJE {
def toJsCmd = "each(function(i) {this.scrollTop=this.scrollHeight;})"
}
+ /**
+ * Bind an event handler to the "click" JavaScript event, or trigger that event on an element.
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

I wonder if there's a benefit to essentially reproducing the jQuery API docs here. Perhaps we should simply mark these as “calls the jQuery click function on a given object” with some top-level docs on JqJE, of the style:

These functions are meant to be combined using the ~> operator. For example:

  JqJE.Jq("button") ~> JqClick(AnonFunc(...))

So JqGetAttr would be “calls the jQuery attr function on a given object with the given attribute name”, etc. It seems like it makes sense to delegate to the jQuery docs on that. We could even link to the jQuery docs on the relevant function. Just a thought, though.

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -178,6 +184,10 @@ object JqJE {
override def toJsCmd = "jQuery(document)"
}
+ /**
+ * Execute a JsCmd when the Char is pressed. Uses jQuery keypress
+ * @param what a tuple of (Char, JsCmd)
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Really what is any number of tuples of Char, JsCmd, and the command executes a set of JsCmds when their corresponding Chars are pressed, right?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -194,6 +204,11 @@ object JqJE {
override def toJsCmd = "jQuery('#'+" + id.toJsCmd + ")"
}
+ /**
+ * Set the value of an attribute key, the value "value"
+ * @param key the attribute to set its value
+ * @param value the value :)
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Obviously a personal choice thing, but it seems like the @params here aren't really adding anything to the main API doc, right?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -194,6 +204,11 @@ object JqJE {
override def toJsCmd = "jQuery('#'+" + id.toJsCmd + ")"
}
+ /**
+ * Set the value of an attribute key, the value "value"
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

I'd say ‘, to the value “value”’

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -238,6 +253,9 @@ object JqJE {
"prependTo(" + fixHtmlFunc("inline", content){str => str} + ")"
}
+ /**
+ * Set the css value of an element
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

In keeping with some of the others, maybe ‘Set the value of the CSS property “name” to the value “value”’?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -318,6 +345,10 @@ object JqJE {
object JqJsCmds {
implicit def jsExpToJsCmd(in: JsExp) = in.cmd
+ /**
+ * Sets the JavScript that will be executed when document is ready
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

As above, I'd consider changing this to “Adds the given cmd to the JavaScript that will be executed when the document is ready”

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
def apply(uid: String) = new Hide(uid, Empty)
+ /**
+ * Hide an element identified by uid and the animation will last @time
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Not sure which approach you prefer, but it seems like the docs for this and Show should follow the same general format, right?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
class Hide(val uid: String, val time: Box[TimeSpan]) extends JsCmd with HasTime {
def toJsCmd = "try{jQuery(" + ("#" + uid).encJs + ").hide(" + timeStr + ");} catch (e) {}"
}
+ /**
+ * Show a message @msg in the id @where for @duration milliseconds and fade out in @fadeout milliseconds
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

I actually kind of like this way of delimiting parameters (the @ prefix), but it seems like at least all of the new API docs should consistently either use @s, or enclose in quotes, or simply refer to the param the same way. Thoughts?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
class Hide(val uid: String, val time: Box[TimeSpan]) extends JsCmd with HasTime {
def toJsCmd = "try{jQuery(" + ("#" + uid).encJs + ").hide(" + timeStr + ");} catch (e) {}"
}
+ /**
+ * Show a message @msg in the id @where for @duration milliseconds and fade out in @fadeout milliseconds
+ *
+ * @param where the id of where to show the message
+ * @param msg the message as a NodeSeq
+ * @param duration show the msessage for @duration in milliseconds or "slow", "fast"
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Interesting. Because duration is a TimeSpan, will it actually accept “slow” or “fast” as values?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -421,16 +503,42 @@ object JqJsCmds {
def apply(id: String) = new FadeIn(id, JsRules.prefadeDuration, JsRules.fadeTime)
}
+ /**
+ * Fade in an element with @id for @duration in milliseconds or "slow", "fast"
+ * and use @fadeTime
+ *
+ * @param id
+ * @param duration
+ * @param fadeTime
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Is it best to drop these since they're empty?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

.../main/scala/net/liftweb/http/js/jquery/JqJsCmds.scala
@@ -421,16 +503,42 @@ object JqJsCmds {
def apply(id: String) = new FadeIn(id, JsRules.prefadeDuration, JsRules.fadeTime)
}
+ /**
+ * Fade in an element with @id for @duration in milliseconds or "slow", "fast"
+ * and use @fadeTime
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Derp. It looks like this waits @duration before fading the element in for @fadetime ms, if I'm not misreading?

@Shadowfiend Shadowfiend commented on the diff Apr 10, 2012

...main/scala/net/liftweb/http/js/yui/YUIArtifacts.scala
@@ -37,27 +37,46 @@ import JE._
* event.js
*/
object YUIArtifacts extends JSArtifacts {
+ /**
+ * Toggles between current JS object and the object denominated by id
@Shadowfiend

Shadowfiend Apr 10, 2012

Owner

Same revisions as above.

I see these are actually just copied in from JsArtifacts though. Is there no way to simply say that we inherit the docs from the thing we're overriding?

Owner

Shadowfiend commented Apr 10, 2012

Heh. I know that's a lot of comments << >>

If I get a chance later today and you prefer, I can try and make most of these changes to this branch. I know I'm an extreme nitpicker when it comes to grammar minutiae and such. Up to you!

Owner

dpp commented Apr 10, 2012

In general, doc changes should be posted directly to master. I'm all in favor of Antonio's nit-picking, but let's just work through the doc changes on master.

@dpp dpp pushed a commit that referenced this pull request Apr 10, 2012

David Pollak Merge pull request #1254 from lift/diego_doc-rec-2
* More API documentation.
68715cd

@dpp dpp merged commit 68715cd into master Apr 10, 2012

Owner

Shadowfiend commented Apr 10, 2012

Cool beans, I'll apply these changes on master when I get a chance. Thanks for the guiding word!

Owner

fmpwizard commented Apr 10, 2012

Thanks David, and thanks Antonio for the comments too.

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