Conversation
|
Note: We should add more doc pages. We actually have none. |
|
ok [IMP] Link: add type information |
dmo-odoo
added
needs refactoring
Zynton
left a comment
Zynton
left a comment
There was a problem hiding this comment.
Sorry for the excessively detailed review but when you write doc for code that I wrote I guess that's to be expected :-) There are also a few notes on form (language mostly).
| @@ -0,0 +1,20 @@ | |||
| # Modifiers | |||
| A modifier is: ... | |||
There was a problem hiding this comment.
?
You could think of a modifier the way you think of a loadable but for nodes: they are simple holders of information that have the potential of modifying the node.
| # Modifiers | ||
| A modifier is: ... | ||
|
|
||
| The main modifiers are `Attribute` and `Format`. |
There was a problem hiding this comment.
Nope. And they are on plugins ^^
|
|
||
| The main modifiers are `Attribute` and `Format`. | ||
|
|
||
| ## Attributes |
| - retrieve HTML attributes that a DOM node has originally | ||
| - render the HTML attributes that a VNode has | ||
|
|
||
| ## Format |
There was a problem hiding this comment.
On Inline Plugin (though that might have to move to core, I don't know).
|
|
||
| ## Attributes | ||
| When parsing and rendering a document, we need to save the attributes that a | ||
| DOM node has and save it in our VNode. |
There was a problem hiding this comment.
- "save them"
- you mention the DOM, that's because indeed this is from XML plugin.
| import { Constructor, isConstructor } from '../../utils/src/utils'; | ||
|
|
||
| /** | ||
| * When parsing dom elements, modifiers can be DOM inline nodes (a, b, strong). |
There was a problem hiding this comment.
Don't write this here.
- No mention of the DOM in
core - No mention of a plugin in
core(Formatin this case) - Start your description of the class by talking about the general case, the general purpose, not the edge cases.
- Be consistent: you write "dom" and "DOM" in the same sentence.
| /** | ||
| * When parsing dom elements, modifiers can be DOM inline nodes (a, b, strong). | ||
| * | ||
| * The order of modifiers are important because it impact the way they will |
There was a problem hiding this comment.
"The order" -> "is"
"It" -> "impacts"
=> "The order of modifiers is important because it may impact their rendering." ?
| * A Format might represent DOM inline nodes (a, b, strong) that were parsed or | ||
| * the DOM inline node (a, b, strong) that we might render. |
There was a problem hiding this comment.
There is a TODO just below: "remove this reference to DOM."
I would try and rewrite this to not invoke the DOM.
Plus, all the uses of "might" make this very hesitant ^^
Now what this does however is reveal how a Format is not fundamentally different from any other Modifier apart from its DOM rendering... @dmo-odoo Any input?
| export class Format extends Modifier { | ||
| htmlTag: string; // TODO: remove this reference to DOM. | ||
| /** | ||
| * The modifiers for a format is `Attribute`. |
There was a problem hiding this comment.
No idea what that sentence is supposed to mean. If you mean that the only modifier a Format can have is Attributes, that's wrong. Modifiers can potentially hold any kind of information to modify a node or a format.
| * Render the current format into a dom Node. | ||
| * | ||
| * Meant to be used with `FormatDomRenderer`. | ||
| */ |
There was a problem hiding this comment.
// TODO: remove this reference to DOM.
(my bad)
3 participants
No description provided.