Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11348255:
Adding docs to Element. (Closed)
DescriptionAdding docs to Element.
This is just some of the docs for the members which are explicit in the class, I'll also be working on more for the generated members.
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=16060
Patch Set 1 #
Total comments: 56
Patch Set 2 : #
Total comments: 45
Patch Set 3 : #
Total comments: 2
Messages
Total messages: 10 (0 generated)
Lots of comments, sorry. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... File sdk/lib/html/templates/html/impl/impl_Element.darttemplate (right): https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:330: * The base class of all components for an HTML document. "... all components of an ..." maybe? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:330: * The base class of all components for an HTML document. Also, what is a "component" exactly? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:335: * Creates an HTML Element from a valid fragment of HTML. I think we've been doing it so the first sentence is its own paragraph. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:344: * and text be added via [text]. Can you add sample code for how this method works? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:351: * This is similar to document.createElement. "[document.createElement]" maybe? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:356: * var element = new DivElement(); Maybe add: * var divElement = new Element.tag('divElement'); * print(divElement is DivElement); // 'true' * var myElement = new Element.tag('myTag'); * print(myElement is UnknownElement); // 'true' https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:362: * Provides access to all attributes on this element. "Static/instance variable and getter/setter descriptions are noun phrases." Should be: "All attributes of this element." https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:365: * Any modifications to the map will automatically be applied to this Maybe "any modifications to the attribute map will ..." https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:366: * element. Can you add details on what "default namespace" means? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:397: * This collection can be used to add and remove elements from the document. Can you add code samples for this method? It's a really important one. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:411: * specified group of selectors. This is another really important method that could use more details. Specifically, code samples for this would be handy, and especially useful would be a "See also:" to the CSS selectors syntax. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:445: * The keys for the map must follow these rules: With all of these rules, it'd be nice to have a "See also:" as to the source of the rules. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:448: * * The name cannot contain any capitol letters. "capital" https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:450: * Any keys from markup will be converted to camel-cased keys in the map I have no idea what this means. "from markup"? Who converts what where? Code samples would be helpful here. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:474: * Gets the final set of all CSS values applied to this element, including Noun phrase. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:474: * Gets the final set of all CSS values applied to this element, including I think "final set" is weird. Final is a keyword, and means something different. I think better would be: "The set of all CSS styles applied to this element, including inherited and default styles." https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:476: * As opposed to the [style] property which contains the values specified Reverse these clauses to focus on computedStyle. "Computed style contains..., as opposed to [style]..." https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:478: * inherited from other sources, such as parent elements or stylesheets. Maybe a see also on inherited styles? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:487: * Similar to [computedStyle] but this targets pseudo-elements such as I think it'd be better to say something like "This returns the computed styles for pseudo-elements such as..." https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:488: * `::after, ::before, ::marker, ::line-marker`. See also on pseudo elements maybe? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:488: * `::after, ::before, ::marker, ::line-marker`. Maybe note that all pseudo elements begin with '::' if they do, or otherwise. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:549: * The [where] parameter indicates: I think this is a clunky way of expressing the enumeration. Maybe ask Kathy as the best way to do this, as I imagine we're going to need to do this elsewhere. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:553: * * `afterEnd` Immediately after this element. Maybe some sample code for this or more details? Does it show up in children for example? https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:567: * Parses [text] as an HTML fragment and inserts it into the DOM at the No [] in the first sentence. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:588: * The [where] parameter indicates: Maybe make the enum as a class with private constructor and fields so that we have instances of that class instead of "String"
Andrei's comments are very good. Just a few things to add. About [someParam] in the descriptions: I'd like to avoid that if we can naturally put the [someParam] elsewhere or just not include [someParam] if its use is obvious. But if it's a one-line doc comment or it's just the best place to refer to [someParam] (AND referring to [someParam] is really necessary), then it's OK to use it in the description. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7345: * Creates an HTML Element from a valid fragment of HTML. Add a blank line after. (Do this globally.) Also, Element -> element (or maybe "HTML Element" -> "element" or "Element") Otherwise, it seems like there might be other kinds of Elements. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7455: * The keys for the map must follow these rules: Add blank lines before and after this line. (Bulleted lists must be preceded by a blank line, sadly.) https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7559: * The [where] parameter indicates: indicates -> indicates where to insert the HTML fragment: (plus see below for other suggestions) https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7583: */ You need a blank line before the bulleted list. How about doing something like this (here and in similar places): /** * Inserts an element into the DOM at the specified location. * * The [where] parameter indicates where to insert [element]: * * * "beforeBegin": Immediately before this element. * * "afterBegin": As the first child of this element. * * "beforeEnd": As the last child of this element. * * "afterEnd": Immediately after this element. */
https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7345: * Creates an HTML Element from a valid fragment of HTML. On 2012/11/27 21:53:42, Kathy Walrath wrote: > Add a blank line after. (Do this globally.) > > Also, Element -> element (or maybe "HTML Element" -> "element" or "Element") > Otherwise, it seems like there might be other kinds of Elements. Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7455: * The keys for the map must follow these rules: On 2012/11/27 21:53:42, Kathy Walrath wrote: > Add blank lines before and after this line. > (Bulleted lists must be preceded by a blank line, sadly.) Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7559: * The [where] parameter indicates: On 2012/11/27 21:53:42, Kathy Walrath wrote: > indicates -> indicates where to insert the HTML fragment: > > (plus see below for other suggestions) Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/dart2js/html_dar... sdk/lib/html/dart2js/html_dart2js.dart:7583: */ On 2012/11/27 21:53:42, Kathy Walrath wrote: > You need a blank line before the bulleted list. How about doing something like > this (here and in similar places): > > /** > * Inserts an element into the DOM at the specified location. > * > * The [where] parameter indicates where to insert [element]: > * > * * "beforeBegin": Immediately before this element. > * * "afterBegin": As the first child of this element. > * * "beforeEnd": As the last child of this element. > * * "afterEnd": Immediately after this element. > */ Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... File sdk/lib/html/templates/html/impl/impl_Element.darttemplate (right): https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:330: * The base class of all components for an HTML document. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > "... all components of an ..." maybe? Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:330: * The base class of all components for an HTML document. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Also, what is a "component" exactly? Reworded some. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:335: * Creates an HTML Element from a valid fragment of HTML. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > I think we've been doing it so the first sentence is its own paragraph. Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:344: * and text be added via [text]. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Can you add sample code for how this method works? Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:351: * This is similar to document.createElement. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > "[document.createElement]" maybe? Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:356: * var element = new DivElement(); On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Maybe add: > > * var divElement = new Element.tag('divElement'); > * print(divElement is DivElement); // 'true' > * var myElement = new Element.tag('myTag'); > * print(myElement is UnknownElement); // 'true' Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:362: * Provides access to all attributes on this element. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > "Static/instance variable and getter/setter descriptions are noun phrases." > > Should be: > "All attributes of this element." Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:365: * Any modifications to the map will automatically be applied to this On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Maybe "any modifications to the attribute map will ..." Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:366: * element. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Can you add details on what "default namespace" means? Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:397: * This collection can be used to add and remove elements from the document. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Can you add code samples for this method? It's a really important one. Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:411: * specified group of selectors. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > This is another really important method that could use more details. > Specifically, code samples for this would be handy, and especially useful would > be a "See also:" to the CSS selectors syntax. Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:445: * The keys for the map must follow these rules: On 2012/11/27 21:19:52, Andrei Mouravski wrote: > With all of these rules, it'd be nice to have a "See also:" as to the source of > the rules. Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:448: * * The name cannot contain any capitol letters. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > "capital" Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:450: * Any keys from markup will be converted to camel-cased keys in the map On 2012/11/27 21:19:52, Andrei Mouravski wrote: > I have no idea what this means. > "from markup"? > Who converts what where? > > Code samples would be helpful here. Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:474: * Gets the final set of all CSS values applied to this element, including On 2012/11/27 21:19:52, Andrei Mouravski wrote: > I think "final set" is weird. Final is a keyword, and means something different. > > I think better would be: > "The set of all CSS styles applied to this element, including inherited and > default styles." Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:476: * As opposed to the [style] property which contains the values specified On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Reverse these clauses to focus on computedStyle. > > "Computed style contains..., as opposed to [style]..." Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:478: * inherited from other sources, such as parent elements or stylesheets. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Maybe a see also on inherited styles? Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:487: * Similar to [computedStyle] but this targets pseudo-elements such as On 2012/11/27 21:19:52, Andrei Mouravski wrote: > I think it'd be better to say something like "This returns the computed styles > for pseudo-elements such as..." Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:488: * `::after, ::before, ::marker, ::line-marker`. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Maybe note that all pseudo elements begin with '::' if they do, or otherwise. They normally do, but there's essentially a fixed list. I'd prefer to rely on the linked documentation for the supported values. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:549: * The [where] parameter indicates: On 2012/11/27 21:19:52, Andrei Mouravski wrote: > I think this is a clunky way of expressing the enumeration. > Maybe ask Kathy as the best way to do this, as I imagine we're going to need to > do this elsewhere. Maybe I should open a bug to add enum support to Dart? Would make it clearer :) https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:553: * * `afterEnd` Immediately after this element. On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Maybe some sample code for this or more details? Does it show up in children for > example? Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:567: * Parses [text] as an HTML fragment and inserts it into the DOM at the On 2012/11/27 21:19:52, Andrei Mouravski wrote: > No [] in the first sentence. Done. https://codereview.chromium.org/11348255/diff/1/sdk/lib/html/templates/html/i... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:588: * The [where] parameter indicates: On 2012/11/27 21:19:52, Andrei Mouravski wrote: > Maybe make the enum as a class with private constructor and fields so that we > have instances of that class instead of "String" http://code.google.com/p/dart/issues/detail?id=7329
lgtm Mostly little things like newlines. Commit when ready! https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... File sdk/lib/html/templates/html/impl/impl_Element.darttemplate (right): https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:352: * An abstract class which all HTML elements extend. Either put a comma before which: "... class, which..." or change which to that: "... class that..." https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:379: * this will create an [UnknownElement]. New line here. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:383: * print(myElement is UnknownElement); // 'true' New line here. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:397: * (such as xlink:href), additional attributes can be accessed via Either code font backticks on xlink:href or quotes around it. Not sure which is more appropriate. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:434: * This collection can be used to add and remove elements from the document. New line before code block. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:457: * Get the first descendant with the class 'classname': Maybe make all of this a code block with "Get the" being comments? Your choice. Either way, newline before "Get the first..." https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:484: * this element. Newline. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:502: * * The name must not begin with `xml`. I think quotes are better for talking about strings instead of codefont. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:506: * Any keys from markup will be converted to camel-cased keys in the map. New line before "For example." https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:507: * For example, HTML specified as: Newlines before and after code blocks. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:555: * This returns the computed styles for pseudo-elements such as I think commas should not be code font. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:631: * * `beforeBegin` Immediately before this element. Quotes instead of code font, I think is better for Strings.
Looking better! I spotted a few things. Have you looked at the generated doc? (It'd be great if I could look at that, just in case there are format issues.) https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7511: * <div data-my-random-value='value'></div>div </div>div -> </div> ? https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7543: * The computedStyle contains the values which are inherited from other which -> that the values -> values? https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7545: * [style] property which contains the values specified directly on this which -> , which (in general, you should either have a comma before "which" -- if removing that phrase wouldn't change the meaning of the word/phrase before -- or change "which" to "that", for clarity) the values -> only the values https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7546: * element, element, -> element. (period) https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7558: * This returns the computed styles for pseudo-elements such as This returns -> Returns https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7572: * Adds the specified element to after the last child of this. this -> this element (seems more natural) https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7579: * Adds the specified text as a text node after the last child of this. this -> this element https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7587: * last child of this. this -> this element https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7614: * * `beforeBegin` Immediately before this element. These bullets seem like they wouldn't look right in the doc (a colon might help here). Also, because these are string values, you should use '. So maybe: * * 'beforeBegin': Immediately... Do the same fix wherever this string is in the doc. Or better yet, document it in just one place and then refer people to that method for the discussion. E.g.: * To see the possible values for [where], read the doc for [insertAdjacentHtml]. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7649: * print(createdElement.classes[0]); // Prints 'something' Perhaps this method should be the one that the main doc AND links to related methods (insertAdjacentText, insertAdjacentElement).
https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7511: * <div data-my-random-value='value'></div>div On 2012/12/12 19:16:06, Kathy Walrath wrote: > </div>div -> </div> > > ? Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7543: * The computedStyle contains the values which are inherited from other On 2012/12/12 19:16:06, Kathy Walrath wrote: > > which -> that > > the values -> values? Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7545: * [style] property which contains the values specified directly on this On 2012/12/12 19:16:06, Kathy Walrath wrote: > > which -> , which > > (in general, you should either have a comma before "which" -- if removing that > phrase wouldn't change the meaning of the word/phrase before -- or change > "which" to "that", for clarity) > > the values -> only the values Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7546: * element, On 2012/12/12 19:16:06, Kathy Walrath wrote: > > element, -> element. > > (period) Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7546: * element, On 2012/12/12 19:16:06, Kathy Walrath wrote: > > element, -> element. > > (period) Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7558: * This returns the computed styles for pseudo-elements such as On 2012/12/12 19:16:06, Kathy Walrath wrote: > > This returns -> Returns Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7572: * Adds the specified element to after the last child of this. On 2012/12/12 19:16:06, Kathy Walrath wrote: > > this -> this element > > (seems more natural) Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7579: * Adds the specified text as a text node after the last child of this. On 2012/12/12 19:16:06, Kathy Walrath wrote: > > this -> this element Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7587: * last child of this. On 2012/12/12 19:16:06, Kathy Walrath wrote: > > this -> this element Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7614: * * `beforeBegin` Immediately before this element. On 2012/12/12 19:16:06, Kathy Walrath wrote: > These bullets seem like they wouldn't look right in the doc (a colon might help > here). Also, because these are string values, you should use '. So maybe: > > * * 'beforeBegin': Immediately... > > Do the same fix wherever this string is in the doc. Or better yet, document it > in just one place and then refer people to that method for the discussion. E.g.: > > * To see the possible values for [where], read the doc for > [insertAdjacentHtml]. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7649: * print(createdElement.classes[0]); // Prints 'something' On 2012/12/12 19:16:06, Kathy Walrath wrote: > > Perhaps this method should be the one that the main doc AND links to related > methods (insertAdjacentText, insertAdjacentElement). Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... File sdk/lib/html/templates/html/impl/impl_Element.darttemplate (right): https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:352: * An abstract class which all HTML elements extend. On 2012/12/12 18:56:17, Andrei Mouravski wrote: > Either put a comma before which: "... class, which..." > or change which to that: "... class that..." Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:379: * this will create an [UnknownElement]. On 2012/12/12 18:56:17, Andrei Mouravski wrote: > New line here. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:383: * print(myElement is UnknownElement); // 'true' On 2012/12/12 18:56:17, Andrei Mouravski wrote: > New line here. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:397: * (such as xlink:href), additional attributes can be accessed via On 2012/12/12 18:56:17, Andrei Mouravski wrote: > Either code font backticks on xlink:href or quotes around it. Not sure which is > more appropriate. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:434: * This collection can be used to add and remove elements from the document. On 2012/12/12 18:56:17, Andrei Mouravski wrote: > New line before code block. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:457: * Get the first descendant with the class 'classname': On 2012/12/12 18:56:17, Andrei Mouravski wrote: > Maybe make all of this a code block with "Get the" being comments? Your choice. > > Either way, newline before "Get the first..." Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:484: * this element. On 2012/12/12 18:56:17, Andrei Mouravski wrote: > Newline. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:502: * * The name must not begin with `xml`. On 2012/12/12 18:56:17, Andrei Mouravski wrote: > I think quotes are better for talking about strings instead of codefont. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:506: * Any keys from markup will be converted to camel-cased keys in the map. On 2012/12/12 18:56:17, Andrei Mouravski wrote: > New line before "For example." Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:507: * For example, HTML specified as: On 2012/12/12 18:56:17, Andrei Mouravski wrote: > Newlines before and after code blocks. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:555: * This returns the computed styles for pseudo-elements such as On 2012/12/12 18:56:17, Andrei Mouravski wrote: > I think commas should not be code font. Done. https://codereview.chromium.org/11348255/diff/5001/sdk/lib/html/templates/htm... sdk/lib/html/templates/html/impl/impl_Element.darttemplate:631: * * `beforeBegin` Immediately before this element. On 2012/12/12 18:56:17, Andrei Mouravski wrote: > Quotes instead of code font, I think is better for Strings. Done.
lgtm
lgtm One tiny thing. https://codereview.chromium.org/11348255/diff/8002/sdk/lib/html/dart2js/html_... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/11348255/diff/8002/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7512: * * The name cannot contain a semi-colon (`;`). Quotes here, too.
https://codereview.chromium.org/11348255/diff/8002/sdk/lib/html/dart2js/html_... File sdk/lib/html/dart2js/html_dart2js.dart (right): https://codereview.chromium.org/11348255/diff/8002/sdk/lib/html/dart2js/html_... sdk/lib/html/dart2js/html_dart2js.dart:7512: * * The name cannot contain a semi-colon (`;`). On 2012/12/12 20:32:19, Andrei Mouravski wrote: > Quotes here, too. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
