Master odoo add prepend nby #226
Conversation
| fromNode.prepend(toNode); | ||
| } | ||
| await this.editor.dispatcher.dispatchHooks('@redraw'); | ||
| } |
There was a problem hiding this comment.
So... the implementation is alright by me, but I have questions regarding the concept. We have moveBefore. Can't it already be used for that?
Like moveBefore(fromDomNode, toDomNode.firstChild) ? If it's specifically for nodes that don't have children yet, then why specifically prepend and not append ?
In any case, if you make a prepend, please also make a append :-)
There was a problem hiding this comment.
The reason is that we do not want to check if there is a child or not.
Goaman
force-pushed
the
master-odoo-add-prepend-nby
branch
4 times, most recently
from
a0ef041
to
743fe86
Compare
Goaman
added
awaiting review
and removed
question
| * | ||
| * @param params | ||
| */ | ||
| async prepend(fromDomNode: Node, toDomNode: Node): Promise<void> { |
There was a problem hiding this comment.
I don't understand what "prepend from" and "prepend to" mean. If I prepend "to a node" then I prepend inside this node ? Then toDomNode is the node in which the node is prepended ? Or I prepend "from" the node ? I have no idea what that means as I think this is not proper english. Obviously, append has the same problem. I suggest node and container for parameters. cc @Zinston if I'm just dumb by not understanding the english of "from" and "to" as prepositions for "prepend".
Goaman
force-pushed
the
master-odoo-add-prepend-nby
branch
from
743fe86
to
3d22437
Compare
| * | ||
| * @param params | ||
| */ | ||
| async prepend(container: Node, node: Node): Promise<void> { |
There was a problem hiding this comment.
We discussed with @Zinston and the only proper way in english is to say "prepend X to Y" so we would switch the order of the parameters. Same for append, obviously.
There was a problem hiding this comment.
I disagree with the english argument. As we saw with sge and the command insertHTML, it is confusing to have to think about what english rule is for any command. It is far simpler to always set the node that will be modified as the first argument as 99% of our helpers.
It make sense to be like english only when it helps rather than create confusion.
There was a problem hiding this comment.
Well actually either way node has to be first... insertHTML has the html to insert as first argument so prepend should have the node to insert as first argument as well.
There was a problem hiding this comment.
No, all other command except insertHTML has the "target" as the first argument. It's insertHTML that should have it's current argument changed.
I need to prepend to an element in Odoo