Skip to content
This repository has been archived by the owner on May 5, 2021. It is now read-only.

Master odoo add prepend nby #226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Master odoo add prepend nby #226

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/plugin-dom-helpers/src/DomHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,32 @@ export class DomHelpers<T extends JWPluginConfig = JWPluginConfig> extends JWPlu
}
await this.editor.dispatcher.dispatchHooks('@redraw');
}
/**
* Prepend a DOM Node to another DOM Node's children.
*
* @param params
*/
async prepend(container: Node, node: Node): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Goaman Goaman Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, all other command except insertHTML has the "target" as the first argument. It's insertHTML that should have it's current argument changed.

const toNodes = this.getNodes(node);
const toNode = toNodes[toNodes.length - 1];
for (const vnode of this.getNodes(container).reverse()) {
vnode.prepend(toNode);
}
await this.editor.dispatcher.dispatchHooks('@redraw');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that we do not want to check if there is a child or not.

/**
* Append a DOM Node to another DOM Node's children.
*
* @param params
*/
async append(container: Node, node: Node): Promise<void> {
const toNodes = this.getNodes(node);
const toNode = toNodes[toNodes.length - 1];
for (const vnode of this.getNodes(container)) {
vnode.append(toNode);
}
await this.editor.dispatcher.dispatchHooks('@redraw');
}
/**
* Insert html content before, after or inside a DOM Node. If no DOM Node
* was provided, empty the range and insert the html content before the it.
Expand Down